Date: Wed, 3 Nov 2010 16:25:28 -0400 From: Jung-uk Kim <jkim@FreeBSD.org> To: John Baldwin <jhb@freebsd.org> Cc: "Moore, Robert" <robert.moore@intel.com>, freebsd-acpi@freebsd.org, Lin Ming <ming.m.lin@intel.com>, Andriy Gapon <avg@freebsd.org> Subject: Re: MacBookPro 5,1 Message-ID: <201011031625.38864.jkim@FreeBSD.org> In-Reply-To: <201011031351.18832.jkim@FreeBSD.org> References: <201010121209.06397.hselasky@c2i.net> <201011031247.56071.jhb@freebsd.org> <201011031351.18832.jkim@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
--Boundary-00=_CVc0MZ2e68ehPVT
Content-Type: text/plain;
charset="iso-8859-1"
Content-Transfer-Encoding: 7bit
Content-Disposition: inline
On Wednesday 03 November 2010 01:51 pm, Jung-uk Kim wrote:
> On Wednesday 03 November 2010 12:47 pm, John Baldwin wrote:
> > On Wednesday, November 03, 2010 12:25:37 pm Jung-uk Kim wrote:
> > > On Wednesday 03 November 2010 08:28 am, John Baldwin wrote:
> > > > On Tuesday, November 02, 2010 6:32:12 pm Jung-uk Kim wrote:
> > > > > On Tuesday 02 November 2010 05:26 pm, John Baldwin wrote:
> > > > > > On Tuesday, November 02, 2010 4:50:18 pm Jung-uk Kim
wrote:
> > > > > > > On Tuesday 02 November 2010 04:24 pm, John Baldwin
wrote:
> > > > > > > > On Tuesday, November 02, 2010 4:14:05 pm Jung-uk Kim
>
> wrote:
> > > > > > > > > On Tuesday 02 November 2010 03:41 pm, John Baldwin
>
> wrote:
> > > > > > > > > > On Tuesday, November 02, 2010 3:29:01 pm Jung-uk
> > > > > > > > > > Kim
> > >
> > > wrote:
> > > > > > > > > > > On Tuesday 02 November 2010 11:29 am, Andriy
> > > > > > > > > > > Gapon
> > >
> > > wrote:
> > > > > > > > > > > > on 29/10/2010 08:51 Andriy Gapon said the
>
> following:
> > > > > > > > > > > > > I guess that a general problem here is that
> > > > > > > > > > > > > it is incorrect to merely use memcpy/bcopy
> > > > > > > > > > > > > to create a copy of a resource if the
> > > > > > > > > > > > > resource has ACPI_RESOURCE_SOURCE field in
> > > > > > > > > > > > > it.
> > > > > > > > > > > >
> > > > > > > > > > > > Hans,
> > > > > > > > > > > >
> > > > > > > > > > > > could you please test the following patch?
> > > > > > > > > > > >
> > > > > > > > > > > > diff --git a/sys/dev/acpica/acpi_pci_link.c
> > > > > > > > > > > > b/sys/dev/acpica/acpi_pci_link.c index
> > > > > > > > > > > > dcf101d..e842635 100644 ---
> > > > > > > > > > > > a/sys/dev/acpica/acpi_pci_link.c +++
> > > > > > > > > > > > b/sys/dev/acpica/acpi_pci_link.c
> > > > > > > > > > > > @@ -767,6 +767,8 @@
> > > > > > > > > > > > acpi_pci_link_srs_from_crs link->l_irq;
> > > > > > > > > > > > else
> > > > > > > > > > > > resptr->Data.ExtendedIrq.Interrupts[0] =
> > > > > > > > > > > > 0;
> > > > > > > > > > > > + memset(&resptr->Data.ExtendedIrq.Resource
> > > > > > > > > > > >So urce , 0, +
> > > > > > > > > > > > sizeof(ACPI_RESOURCE_SOURCE)); link++;
> > > > > > > > > > > > i++;
> > > > > > > > > > > > break;
> > > > > > > > > > >
> > > > > > > > > > > Hmm... Very interesting. Can you please try
> > > > > > > > > > > this, too?
> > > > > > > > > >
> > > > > > > > > > Linux doesn't set the resource source bits up at
> > > > > > > > > > all when doing _SRS, so I'd rather just do that.
> > > > > > > > > > I think what I'd prefer is that we not use the
> > > > > > > > > > prs_template, perhaps just save the type of the
> > > > > > > > > > resource and build a new resource object from
> > > > > > > > > > scratch where the resource is zero'd, the
> > > > > > > > > > appropriate bits are set and then that resource
> > > > > > > > > > is appended to the buffer being built.
> > > > > > > > >
> > > > > > > > > "Linux doesn't do it" is wrong if I am reading the
> > > > > > > > > spec. correctly, i.e., _CRS, _PRS and _SRS must
> > > > > > > > > have the same format and size.
> > > > > > > >
> > > > > > > > Umm, but we aren't setting up the raw bits for _SRS.
> > > > > > > > We are creating a list of ACPI_RESOURCE objects that
> > > > > > > > ACPICA then encodes into a buffer to send to _SRS.
> > > > > > >
> > > > > > > Yes, I understand. However, ACPICA is expecting the
> > > > > > > same size of buffer *including* the optional parts if I
> > > > > > > am reading the code right. Besides, I don't think there
> > > > > > > is any harm in doing the right thing. ;-)
> > > > > >
> > > > > > To be clear, I am suggesting to take an ACPI_RESOURCE
> > > > > > object, bzero it, then set the type and the IRQ and
> > > > > > that's it. Leave the ResourceSource bits as zero. The
> > > > > > size will still be set based on the actual type (or if
> > > > > > needed we can use the cached size from the template copy
> > > > > > we save from _PRS). The point would be to start from a
> > > > > > zero structure instead of from a copy of what we got from
> > > > > > _PRS.
> > > > >
> > > > > It may work if we don't use l_prs_template.
> > > >
> > > > Well, we still need much of the info from the _PRS resource
> > > > (the type, etc.), but I think we should not blindly use the
> > > > template directly when building the buffer for _SRS.
> > >
> > > Actually, I think we should get the information directly from
> > > _CRS as ACPI spec. is suggesting.
> >
> > I would be fine with that, but that does not work if _CRS doesn't
> > work (the acpi_pci_link_srs_from_links() case).
>
> For that case, we must use the template, of course. In fact, my
> patch is more useful for this particular case. :-)
>
> > Are we allowed to modify the buffer ACPICA gives us from _CRS and
> > then pass that back to _SRS?
>
> I believe so. If we go with that route, we don't have to worry
> about ResourceSource.StringPtr or acpi_AppendBufferResource()
> copying stale pointers.
Please see the attached patch. It seems working fine. :-)
Note I had to adjust resource length to prevent reading/writing beyond
buffer size. It should work okay for acpi_pci_link_srs_from_links()
case, I believe. It's a hack anyway. ;-)
Jung-uk Kim
--Boundary-00=_CVc0MZ2e68ehPVT
Content-Type: text/plain; charset="iso-8859-1"; name="acpi_pci_link2.diff"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
filename="acpi_pci_link2.diff"
--- sys/dev/acpica/acpi_pci_link.c 2010-03-05 15:07:53.000000000 -0500
+++ sys/dev/acpica/acpi_pci_link.c 2010-11-03 16:09:40.000000000 -0400
@@ -268,6 +268,7 @@ link_add_crs(ACPI_RESOURCE *res, void *c
static ACPI_STATUS
link_add_prs(ACPI_RESOURCE *res, void *context)
{
+ ACPI_RESOURCE *tmp;
struct link_res_request *req;
struct link *link;
UINT8 *irqs = NULL;
@@ -321,8 +322,17 @@ link_add_prs(ACPI_RESOURCE *res, void *c
* Stash a copy of the resource for later use when doing
* _SRS.
*/
- bcopy(res, &link->l_prs_template, sizeof(ACPI_RESOURCE));
+ tmp = &link->l_prs_template;
+ bcopy(res, tmp, sizeof(*res));
if (is_ext_irq) {
+ /*
+ * XXX acpi_AppendBufferResource() cannot handle
+ * optional ResourceSource.
+ */
+ bzero(&tmp->Data.ExtendedIrq.ResourceSource,
+ sizeof(tmp->Data.ExtendedIrq.ResourceSource));
+ tmp->Length = sizeof(tmp->Data.ExtendedIrq);
+
link->l_num_irqs =
res->Data.ExtendedIrq.InterruptCount;
ext_irqs = res->Data.ExtendedIrq.Interrupts;
@@ -688,18 +698,17 @@ acpi_pci_link_add_reference(device_t dev
static ACPI_STATUS
acpi_pci_link_srs_from_crs(struct acpi_pci_link_softc *sc, ACPI_BUFFER *srsbuf)
{
- ACPI_RESOURCE *resource, *end, newres, *resptr;
- ACPI_BUFFER crsbuf;
+ ACPI_RESOURCE *end, *res;
ACPI_STATUS status;
struct link *link;
int i, in_dpf;
/* Fetch the _CRS. */
ACPI_SERIAL_ASSERT(pci_link);
- crsbuf.Pointer = NULL;
- crsbuf.Length = ACPI_ALLOCATE_BUFFER;
- status = AcpiGetCurrentResources(acpi_get_handle(sc->pl_dev), &crsbuf);
- if (ACPI_SUCCESS(status) && crsbuf.Pointer == NULL)
+ srsbuf->Pointer = NULL;
+ srsbuf->Length = ACPI_ALLOCATE_BUFFER;
+ status = AcpiGetCurrentResources(acpi_get_handle(sc->pl_dev), srsbuf);
+ if (ACPI_SUCCESS(status) && srsbuf->Pointer == NULL)
status = AE_NO_MEMORY;
if (ACPI_FAILURE(status)) {
if (bootverbose)
@@ -710,14 +719,13 @@ acpi_pci_link_srs_from_crs(struct acpi_p
}
/* Fill in IRQ resources via link structures. */
- srsbuf->Pointer = NULL;
link = sc->pl_links;
i = 0;
in_dpf = DPF_OUTSIDE;
- resource = (ACPI_RESOURCE *)crsbuf.Pointer;
- end = (ACPI_RESOURCE *)((char *)crsbuf.Pointer + crsbuf.Length);
+ res = (ACPI_RESOURCE *)srsbuf->Pointer;
+ end = (ACPI_RESOURCE *)((char *)srsbuf->Pointer + srsbuf->Length);
for (;;) {
- switch (resource->Type) {
+ switch (res->Type) {
case ACPI_RESOURCE_TYPE_START_DEPENDENT:
switch (in_dpf) {
case DPF_OUTSIDE:
@@ -731,67 +739,44 @@ acpi_pci_link_srs_from_crs(struct acpi_p
__func__);
break;
}
- resptr = NULL;
break;
case ACPI_RESOURCE_TYPE_END_DEPENDENT:
/* We are finished with DPF parsing. */
KASSERT(in_dpf != DPF_OUTSIDE,
("%s: end dpf when not parsing a dpf", __func__));
in_dpf = DPF_OUTSIDE;
- resptr = NULL;
break;
case ACPI_RESOURCE_TYPE_IRQ:
MPASS(i < sc->pl_num_links);
- MPASS(link->l_prs_template.Type == ACPI_RESOURCE_TYPE_IRQ);
- newres = link->l_prs_template;
- resptr = &newres;
- resptr->Data.Irq.InterruptCount = 1;
+ res->Data.Irq.InterruptCount = 1;
if (PCI_INTERRUPT_VALID(link->l_irq)) {
KASSERT(link->l_irq < NUM_ISA_INTERRUPTS,
("%s: can't put non-ISA IRQ %d in legacy IRQ resource type",
__func__, link->l_irq));
- resptr->Data.Irq.Interrupts[0] = link->l_irq;
+ res->Data.Irq.Interrupts[0] = link->l_irq;
} else
- resptr->Data.Irq.Interrupts[0] = 0;
+ res->Data.Irq.Interrupts[0] = 0;
link++;
i++;
break;
case ACPI_RESOURCE_TYPE_EXTENDED_IRQ:
MPASS(i < sc->pl_num_links);
- MPASS(link->l_prs_template.Type == ACPI_RESOURCE_TYPE_EXTENDED_IRQ);
- newres = link->l_prs_template;
- resptr = &newres;
- resptr->Data.ExtendedIrq.InterruptCount = 1;
+ res->Data.ExtendedIrq.InterruptCount = 1;
if (PCI_INTERRUPT_VALID(link->l_irq))
- resptr->Data.ExtendedIrq.Interrupts[0] =
+ res->Data.ExtendedIrq.Interrupts[0] =
link->l_irq;
else
- resptr->Data.ExtendedIrq.Interrupts[0] = 0;
+ res->Data.ExtendedIrq.Interrupts[0] = 0;
link++;
i++;
break;
- default:
- resptr = resource;
}
- if (resptr != NULL) {
- status = acpi_AppendBufferResource(srsbuf, resptr);
- if (ACPI_FAILURE(status)) {
- device_printf(sc->pl_dev,
- "Unable to build resources: %s\n",
- AcpiFormatException(status));
- if (srsbuf->Pointer != NULL)
- AcpiOsFree(srsbuf->Pointer);
- AcpiOsFree(crsbuf.Pointer);
- return (status);
- }
- }
- if (resource->Type == ACPI_RESOURCE_TYPE_END_TAG)
+ if (res->Type == ACPI_RESOURCE_TYPE_END_TAG)
break;
- resource = ACPI_NEXT_RESOURCE(resource);
- if (resource >= end)
+ res = ACPI_NEXT_RESOURCE(res);
+ if (res >= end)
break;
}
- AcpiOsFree(crsbuf.Pointer);
return (AE_OK);
}
--Boundary-00=_CVc0MZ2e68ehPVT--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201011031625.38864.jkim>