Re: [PATCH] ACPI / scan: Allow ACPI drivers to bind to PNP device objects

[Zhang Rui]

On Fri, 2014-08-22 at 19:53 +0200, Rafael J. Wysocki wrote:
> On Friday, August 22, 2014 10:00:31 AM Zhang Rui wrote:
> > On Thu, 2014-08-21 at 19:10 +0200, Rafael J. Wysocki wrote:
> > > On Thursday, August 21, 2014 08:08:54 PM Zhang Rui wrote:
> > > > Hi, Rafael,
> > > > 
> > > > On Thu, 2014-08-21 at 06:04 +0200, Rafael J. Wysocki wrote:
> > > > > From: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
> > > 
> > > [cut]
> > > 
> > > > Note that I've just tested on my machine and it works well.
> > > > I still need the bug reporter to check if the patch fixes bug 81511 or not.
> > > 
> > > The FUJ02B1 and FUJ02E3 devices in bug 81971 have the same problem and
> > > they aren't motherboard devices.
> > 
> > Right, but IMO, the rootcause of that bug is that
> > 1. the PNP id table in fujitsu-laptop driver was introduced for some
> > reason, probably it is used as an indicator for module auto-loading, and
> > nowadays, this is redundant because fujitsu-laptop driver probes ACPI
> > device only, and the driver will be loaded if the ACPI device objects
> > for FUJ02B1 and FUJ02E3 is created.
> 
> We may be able to drop the Fujitsu devices from the ACPI PNP list and all may
> work.  Still, does that fix all of the potential problems?
> 
> > 2. This "redundant" PNP id table results in that those IDs are added to
> > PNP ID list unnecessarily, and results in PNP device nodes for those
> > devices are created unnecessarily.
> 
> Yes, that may be the case, but the way to deal with that is not to break
> thing and then see what's broken and fix it, but to get rid of ACPI drivers
> one by one in which case we can control what's been changed and why.
> 
> > >   Yes, we need to convert that driver
> > > to use a PNP driver structure or a platform device, but (1) we need a
> > > -stable fix *first*
> > 
> > I agree.
> > 
> > >  and (2) the cases we already know about may not be
> > > the only broken ones.
> > 
> > Agree.
> > But the issue addressed in your patch is that PNP scan handler blocks
> > ACPI driver from being probed, right?
> 
> Yes.
> 
> > So my question would be,
> > 1. If the id in PNP scan handler does not have a PNP driver, like the
> >    FUJ02B1/FUJ02E3 issue, what do we need the id in PNP scan handler?
> >    In fact, I think this is a good chance for us to cleanup the ACPI PNP
> >    id list, as long as we can fix them in time.
> 
> No.
> 
> We need -stable to work and there's no way I will mark the motherboard
> resource changes for -stable.

I agree. And I would rather consider my patch as the start point of a
long term solution.

>   Second, if we deal with it as I said (that is,
> get rid of ACPI drivers gradually), we will clean up that list in the process
> without breaking things for people in random ways.

Agree.
> 
> > 2. If the id in PNP scan handler has a PNP driver, should we allow both
> >    PNP driver and ACPI driver loaded? I think PNP system driver is the
> >    only case that makes us have to say yes, and what I'm trying to do
> >    is to fix this in the following patch.
> > 
> > Plus, IMO, your patch only fixes the PNP bus vs. ACPI bus issue. We
> > still may get bug report complaining some *PLATFORM* driver stops to
> > functional if the ACPI node has _CID PNP0C01/PNP0C02, sooner or later.
> > right?
> 
> No.
> 
> We never allowed ACPI drivers to bind to ACPI device objects having platform
> device companions,

No, I mean, a platform device is used to be created as the ACPI device
object _HID was in the previous acpi_platform scan handler list, but if
this device has _CID PNP0C01/PNP0C02, the platform device will not be
created any more after the ACPI enumeration re-work patches.

>  wherease we *did* allow that for ACPI device objects having
> PNP device companions.  We simply need to go back to what we were doing and fix
> things *on* *top* of that.
> 
> Any other approach pretty much guarantees leaving breakage in random places.
> 
> So I'm fine with cleaning up the PNP list *if* you convert the drivers in
> question from ACPI drivers to something else (platform drivers preferably)
> at the same time.
> 
yes, I see.

So I guess the following patch can be upstream candidate, right?