Re: mss.c pcm fix to ' attach returned 6 ' load failure for v5.x acpi and up

On Tuesday 12 July 2005 03:19 pm, Harry Coin wrote:
> At 10:44 AM 7/12/2005 -0400, John Baldwin wrote:
> >Now, when ACPI is enabled, the
> >pnpmss driver doesn't probe the device IIRC?
>
> Both the mss_probe and pnpmss_probe routine are called in the ACPI case and
> in the ACPI disabled case.   Let me help by providing proof from some
> edited short form commented dmesg excerpts from upstream postings using
> unmodified-except-for-debug-writes mss.c:

I understand that.  My last patch (which you apparently didn't test), makes 
the non-pnp mss probe not be used by ACPI ever.

> The question I can't get out of my head is this:  When I change the mss.c
> code to use the routine the manual says non-PNP devices are supposed to
> use: ISA_PNP_PROBE -- all bootup operations are correct in both the ACPI
> and non ACPI case.
>
> Why doesn't that count as 'fixed, update mss.c and lets move on?'.

Because there are 20-30 _other_ drivers that use isa_get_logicalid(), and if 
there's a bug in isa_get_logicalid() then it's going to break _all_ those 
drivers, and I'd rather fix ACPI than have to wade through a whole bunch of 
different drivers.  Also, having to use an empty PNP_PROBE list is a rather 
lame solution.

> Clearly the present code uses another function that doesn't work in the
> ACPI case.  Shouldn't the investigation focus on what ISA_PNP_PROBE knows
> that the other call doesn't?   ISA_PNP_PROBE in mss_probe works and matches
> the docs, the isa_get_whatziz doesn't work and is not accepted as correct
> practice in the docs.   Maybe the doc writer has the answer.  At any
> rate  I don't get why this further investigation when an apparently
> 'canonical' practice works.  That, or the architecture manual needs
> changing to mention this other way to avoid wrongful probing by non-PNP isa
> drivers when called by ACPI or isa0.

The problem is that ISA_PNP_PROBE() just uses isa_get_logicalid() (basically).  
Here's what the two functions look like in ACPI:

/* Probe _HID and _CID for compatible ISA PNP ids. */
static uint32_t
acpi_isa_get_logicalid(device_t dev)
{
    ACPI_DEVICE_INFO	*devinfo;
    ACPI_BUFFER		buf;
    ACPI_HANDLE		h;
    ACPI_STATUS		error;
    u_int32_t		pnpid;
    int			i;

    ACPI_FUNCTION_TRACE((char *)(uintptr_t)__func__);

    pnpid = 0;
    buf.Pointer = NULL;
    buf.Length = ACPI_ALLOCATE_BUFFER;

    /* Fetch and validate the HID. */
    if ((h = acpi_get_handle(dev)) == NULL)
	goto out;
    error = AcpiGetObjectInfo(h, &buf);
    if (ACPI_FAILURE(error))
	goto out;
    devinfo = (ACPI_DEVICE_INFO *)buf.Pointer;

    if ((devinfo->Valid & ACPI_VALID_HID) != 0)
	pnpid = PNP_EISAID(devinfo->HardwareId.Value);

out:
    if (buf.Pointer != NULL)
	AcpiOsFree(buf.Pointer);
    return_VALUE (pnpid);
}

static int
acpi_isa_pnp_probe(device_t bus, device_t child, struct isa_pnp_id *ids)
{
    int			result, cid_count, i;
    uint32_t		lid, cids[8];

    ACPI_FUNCTION_TRACE((char *)(uintptr_t)__func__);

    /*
     * ISA-style drivers attached to ACPI may persist and
     * probe manually if we return ENOENT.  We never want
     * that to happen, so don't ever return it.
     */
    result = ENXIO;

    /* Scan the supplied IDs for a match */
    lid = acpi_isa_get_logicalid(child);
    cid_count = acpi_isa_get_compatid(child, cids, 8);
    while (ids && ids->ip_id) {
	if (lid == ids->ip_id) {
	    result = 0;
	    goto out;
	}
	for (i = 0; i < cid_count; i++) {
	    if (cids[i] == ids->ip_id) {
		result = 0;
		goto out;
	    }
	}
	ids++;
    }

 out:
    if (result == 0 && ids->ip_desc)
	device_set_desc(child, ids->ip_desc);

    return_VALUE (result);
}

The other thing to keep in mind here is that your soundcard isn't an ACPI 
device that this code would know about, it's an ISA PnP device which is 
completely different (hung off of isa0 for one), and that what is happening 
is that ACPI devices like acpi_tz0 (thermal zone) are being probed by 
mss_probe() and it's not rejecting them because it thinks logicalid() is 
zero.  Hmm, I bet I know what it is actually.  ACPI has HIDs that are strings 
like ACPI003 for things like thermal zones and batteries rather than an ESIA 
encoded PNPxxx string.  So even though there is a valid HID, PNP_EISAID() 
ends up returning 0.  *sigh*  The other thing is that the non-pnp mss probe 
simply does not need to be hooked up to the ACPI bus at all since ACPI will 
only enumerate PnP devices.  I would appreciate it if you would try the 
simple patch I posted previously to remove the one line from mss.c that hooks 
the non-pnp mss driver up to acpi.

-- 
John Baldwin <jhb_at_FreeBSD.org>  <><  http://www.FreeBSD.org/~jhb/
"Power Users Use the Power to Serve"  =  http://www.FreeBSD.org