| Message ID | 20180809091558.4317-2-hdegoede@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | i2c-multi-instantiate pseudo driver | expand |
On Thu, Aug 9, 2018 at 11:15 AM, Hans de Goede <hdegoede@redhat.com> wrote: > Since commit 63347db0affa ("ACPI / scan: Use acpi_bus_get_status() to > initialize ACPI_TYPE_DEVICE devs") the status field of normal acpi_devices > gets set to 0 by acpi_bus_type_and_status() and filled with its actual > value later when acpi_add_single_object() calls acpi_bus_get_status(). > > This means that any acpi_match_device_ids() calls in between will always > fail with -ENOENT. > > We already have a workaround for this, which temporary forces status to > ACPI_STA_DEFAULT in drivers/acpi/x86/utils.c: acpi_device_always_present() > and the next commit in this series adds another acpi_match_device_ids() > call between status being initialized as 0 and the acpi_bus_get_status() > call. > > Rather then adding another workaround, this commit makes > acpi_bus_type_and_status() initialize status to ACPI_STA_DEFAULT, this is > safe to do as the only code looking at status between the initialization > and the acpi_bus_get_status() call is those acpi_match_device_ids() calls. > > Note this does mean that we need to (re)set status to 0 in case the > acpi_bus_get_status() call fails. > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > Changes in v3: > -New patch in v3 of this patch-set > > Changes in v4: > -This is not a fix for acpi_is_indirect_io_slave() as I thought at first, > acpi_is_indirect_io_slave() calls acpi_match_device_ids() on its parent > device, where status is already set properly. Rewrite the commit message > accordingly. I've applied the v4 of this patch and I don't think there are any changes from it here. As for the rest of the series I'll wait from comments from Wolfram and the other reviewers. Thanks, Rafael
Hi, On 09-08-18 11:35, Rafael J. Wysocki wrote: > On Thu, Aug 9, 2018 at 11:15 AM, Hans de Goede <hdegoede@redhat.com> wrote: >> Since commit 63347db0affa ("ACPI / scan: Use acpi_bus_get_status() to >> initialize ACPI_TYPE_DEVICE devs") the status field of normal acpi_devices >> gets set to 0 by acpi_bus_type_and_status() and filled with its actual >> value later when acpi_add_single_object() calls acpi_bus_get_status(). >> >> This means that any acpi_match_device_ids() calls in between will always >> fail with -ENOENT. >> >> We already have a workaround for this, which temporary forces status to >> ACPI_STA_DEFAULT in drivers/acpi/x86/utils.c: acpi_device_always_present() >> and the next commit in this series adds another acpi_match_device_ids() >> call between status being initialized as 0 and the acpi_bus_get_status() >> call. >> >> Rather then adding another workaround, this commit makes >> acpi_bus_type_and_status() initialize status to ACPI_STA_DEFAULT, this is >> safe to do as the only code looking at status between the initialization >> and the acpi_bus_get_status() call is those acpi_match_device_ids() calls. >> >> Note this does mean that we need to (re)set status to 0 in case the >> acpi_bus_get_status() call fails. >> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >> --- >> Changes in v3: >> -New patch in v3 of this patch-set >> >> Changes in v4: >> -This is not a fix for acpi_is_indirect_io_slave() as I thought at first, >> acpi_is_indirect_io_slave() calls acpi_match_device_ids() on its parent >> device, where status is already set properly. Rewrite the commit message >> accordingly. > > I've applied the v4 of this patch and I don't think there are any > changes from it here. Correct, there were only changes to the 4th patch in the series. > As for the rest of the series I'll wait from comments from Wolfram and > the other reviewers. Ok, note if you've taken patch 1 you may also want to take patch 3 which is an ACPI code cleanup made possible by patch 1 and otherwise is unrelated. Regards, Hans
On Thu, Aug 9, 2018 at 12:39 PM, Hans de Goede <hdegoede@redhat.com> wrote: > On 09-08-18 11:35, Rafael J. Wysocki wrote: >> On Thu, Aug 9, 2018 at 11:15 AM, Hans de Goede <hdegoede@redhat.com> >> wrote: >> I've applied the v4 of this patch and I don't think there are any >> changes from it here. > > > Correct, there were only changes to the 4th patch in the series. > >> As for the rest of the series I'll wait from comments from Wolfram and >> the other reviewers. > > > Ok, note if you've taken patch 1 you may also want to take patch 3 which > is an ACPI code cleanup made possible by patch 1 and otherwise is > unrelated. I'm under impression Rafael is going to take entire series (at least for patch 4 I'm expecting to give an Ack).
Hi, On 09-08-18 11:51, Andy Shevchenko wrote: > On Thu, Aug 9, 2018 at 12:39 PM, Hans de Goede <hdegoede@redhat.com> wrote: >> On 09-08-18 11:35, Rafael J. Wysocki wrote: >>> On Thu, Aug 9, 2018 at 11:15 AM, Hans de Goede <hdegoede@redhat.com> >>> wrote: > >>> I've applied the v4 of this patch and I don't think there are any >>> changes from it here. >> >> >> Correct, there were only changes to the 4th patch in the series. >> >>> As for the rest of the series I'll wait from comments from Wolfram and >>> the other reviewers. >> >> >> Ok, note if you've taken patch 1 you may also want to take patch 3 which >> is an ACPI code cleanup made possible by patch 1 and otherwise is >> unrelated. > > I'm under impression Rafael is going to take entire series (at least > for patch 4 I'm expecting to give an Ack). As I mentioned in the coverletter, my idea was to have Rafael take patches 1-3 and then merge the 4th patch through the platform/x86 tree. There are only runtime dependencies between the 2 parts and merging them independently should not cause any issues. Regards, Hans
On Thu, Aug 9, 2018 at 11:58 AM, Hans de Goede <hdegoede@redhat.com> wrote: > Hi, > > On 09-08-18 11:51, Andy Shevchenko wrote: >> >> On Thu, Aug 9, 2018 at 12:39 PM, Hans de Goede <hdegoede@redhat.com> >> wrote: >>> >>> On 09-08-18 11:35, Rafael J. Wysocki wrote: >>>> >>>> On Thu, Aug 9, 2018 at 11:15 AM, Hans de Goede <hdegoede@redhat.com> >>>> wrote: >> >> >>>> I've applied the v4 of this patch and I don't think there are any >>>> changes from it here. >>> >>> >>> >>> Correct, there were only changes to the 4th patch in the series. >>> >>>> As for the rest of the series I'll wait from comments from Wolfram and >>>> the other reviewers. >>> >>> >>> >>> Ok, note if you've taken patch 1 you may also want to take patch 3 which >>> is an ACPI code cleanup made possible by patch 1 and otherwise is >>> unrelated. >> >> >> I'm under impression Rafael is going to take entire series (at least >> for patch 4 I'm expecting to give an Ack). > > > As I mentioned in the coverletter, my idea was to have Rafael take > patches 1-3 and then merge the 4th patch through the platform/x86 > tree. There are only runtime dependencies between the 2 parts and > merging them independently should not cause any issues. I can apply the 4th one too if it is ACKed by everyone with a vested interest.
> As for the rest of the series I'll wait from comments from Wolfram and > the other reviewers. Well, I acked patch 4 now because I like the general approach. I can't say much about the rest since this is mostly ACPI/FW related. Thanks Hans, for keeping at it.
Hi, On 09-08-18 11:59, Rafael J. Wysocki wrote: > On Thu, Aug 9, 2018 at 11:58 AM, Hans de Goede <hdegoede@redhat.com> wrote: >> Hi, >> >> On 09-08-18 11:51, Andy Shevchenko wrote: >>> >>> On Thu, Aug 9, 2018 at 12:39 PM, Hans de Goede <hdegoede@redhat.com> >>> wrote: >>>> >>>> On 09-08-18 11:35, Rafael J. Wysocki wrote: >>>>> >>>>> On Thu, Aug 9, 2018 at 11:15 AM, Hans de Goede <hdegoede@redhat.com> >>>>> wrote: >>> >>> >>>>> I've applied the v4 of this patch and I don't think there are any >>>>> changes from it here. >>>> >>>> >>>> >>>> Correct, there were only changes to the 4th patch in the series. >>>> >>>>> As for the rest of the series I'll wait from comments from Wolfram and >>>>> the other reviewers. >>>> >>>> >>>> >>>> Ok, note if you've taken patch 1 you may also want to take patch 3 which >>>> is an ACPI code cleanup made possible by patch 1 and otherwise is >>>> unrelated. >>> >>> >>> I'm under impression Rafael is going to take entire series (at least >>> for patch 4 I'm expecting to give an Ack). >> >> >> As I mentioned in the coverletter, my idea was to have Rafael take >> patches 1-3 and then merge the 4th patch through the platform/x86 >> tree. There are only runtime dependencies between the 2 parts and >> merging them independently should not cause any issues. > > I can apply the 4th one too if it is ACKed by everyone with a vested interest. That works for me, note I'm about to send out a v6 (with only changes to the 4th patch), so hold of a bit with merging this please. Andy does your ack for the 4th patch mean you're ok with Rafael merging this? Regards, Hans
On Thu, 2018-08-09 at 13:36 +0200, Hans de Goede wrote: > Andy does your ack for the 4th patch mean you're ok with Rafael > merging > this? Yes.
diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c index 970dd87d347c..6799d00dd790 100644 --- a/drivers/acpi/scan.c +++ b/drivers/acpi/scan.c @@ -1612,7 +1612,8 @@ static int acpi_add_single_object(struct acpi_device **child, * Note this must be done before the get power-/wakeup_dev-flags calls. */ if (type == ACPI_BUS_TYPE_DEVICE) - acpi_bus_get_status(device); + if (acpi_bus_get_status(device) < 0) + acpi_set_device_status(device, 0); acpi_bus_get_power_flags(device); acpi_bus_get_wakeup_device_flags(device); @@ -1690,7 +1691,7 @@ static int acpi_bus_type_and_status(acpi_handle handle, int *type, * acpi_add_single_object updates this once we've an acpi_device * so that acpi_bus_get_status' quirk handling can be used. */ - *sta = 0; + *sta = ACPI_STA_DEFAULT; break; case ACPI_TYPE_PROCESSOR: *type = ACPI_BUS_TYPE_PROCESSOR;
Since commit 63347db0affa ("ACPI / scan: Use acpi_bus_get_status() to initialize ACPI_TYPE_DEVICE devs") the status field of normal acpi_devices gets set to 0 by acpi_bus_type_and_status() and filled with its actual value later when acpi_add_single_object() calls acpi_bus_get_status(). This means that any acpi_match_device_ids() calls in between will always fail with -ENOENT. We already have a workaround for this, which temporary forces status to ACPI_STA_DEFAULT in drivers/acpi/x86/utils.c: acpi_device_always_present() and the next commit in this series adds another acpi_match_device_ids() call between status being initialized as 0 and the acpi_bus_get_status() call. Rather then adding another workaround, this commit makes acpi_bus_type_and_status() initialize status to ACPI_STA_DEFAULT, this is safe to do as the only code looking at status between the initialization and the acpi_bus_get_status() call is those acpi_match_device_ids() calls. Note this does mean that we need to (re)set status to 0 in case the acpi_bus_get_status() call fails. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- Changes in v3: -New patch in v3 of this patch-set Changes in v4: -This is not a fix for acpi_is_indirect_io_slave() as I thought at first, acpi_is_indirect_io_slave() calls acpi_match_device_ids() on its parent device, where status is already set properly. Rewrite the commit message accordingly. --- drivers/acpi/scan.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)