| Message ID | 20180207145610.88434-1-andriy.shevchenko@linux.intel.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
On Wed, Feb 7, 2018 at 3:56 PM, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > Instead of playing tricks with last invalid entry, > return simple -ENODATA error code cast to pointer. > > It would be good for future in case caller passes NULL pointer for > ID table. Moreover, caller can check the code to be sure what happened > inside callee. > > Fixes: 2b9c698efa58 ("ACPI / scan: Take the PRP0001 position in the list of IDs into account") I still don't think the Fixes: tag here is valid (the code works as is AFAICS), but I could drop it when applying the patch just fine. :-) That said, see below. > Cc: Sinan Kaya <okaya@codeaurora.org> > Cc: Sakari Ailus <sakari.ailus@linux.intel.com> > Cc: Vinod Koul <vinod.koul@intel.com> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > --- > drivers/acpi/bus.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c > index 58239abb0a72..761286e50067 100644 > --- a/drivers/acpi/bus.c > +++ b/drivers/acpi/bus.c > @@ -769,7 +769,7 @@ static const struct acpi_device_id *__acpi_match_device( > */ > if (!strcmp(ACPI_DT_NAMESPACE_HID, hwid->id) > && acpi_of_match_device(device, of_ids)) > - return id; > + return ERR_PTR(-ENODATA); Doesn't the comment above need to be updated? Also the return value here means "success", so why is an error the right choice? > } > return NULL; > } > -- Overall, this really looks like a preparation for a future patch, so why not just say that straight away in the changelog? -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2018-02-08 at 16:14 +0100, Rafael J. Wysocki wrote: > On Wed, Feb 7, 2018 at 3:56 PM, Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: > > Instead of playing tricks with last invalid entry, > > return simple -ENODATA error code cast to pointer. > > > > It would be good for future in case caller passes NULL pointer for > > ID table. Moreover, caller can check the code to be sure what > > happened > > inside callee. > > > > Fixes: 2b9c698efa58 ("ACPI / scan: Take the PRP0001 position in the > > list of IDs into account") > > I still don't think the Fixes: tag here is valid (the code works as is > AFAICS), but I could drop it when applying the patch just fine. :-) OK. > > if (!strcmp(ACPI_DT_NAMESPACE_HID, hwid->id) > > && acpi_of_match_device(device, of_ids)) > > - return id; > > + return ERR_PTR(-ENODATA); > > Doesn't the comment above need to be updated? Will do. > Also the return value here means "success", so why is an error the > right choice? Because we need to return something which is not NULL. Naturally feels the error code, esp. ENODATA, is quite suitable. We indeed have no data in this case, and it's not a NULL case (not found / not match) — we have a match. > Overall, this really looks like a preparation for a future patch, so > why not just say that straight away in the changelog? It's not _just_ a preparation, it mitigates the trick used in mentioned by Fixes tag commit. I would rather update comment here, and add explanation to the commit message to be sure it covers tricks mitigation and preparation purposes.
On Thu, Feb 8, 2018 at 4:14 PM, Rafael J. Wysocki <rafael@kernel.org> wrote: > On Wed, Feb 7, 2018 at 3:56 PM, Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: >> Instead of playing tricks with last invalid entry, >> return simple -ENODATA error code cast to pointer. >> >> It would be good for future in case caller passes NULL pointer for >> ID table. Moreover, caller can check the code to be sure what happened >> inside callee. >> >> Fixes: 2b9c698efa58 ("ACPI / scan: Take the PRP0001 position in the list of IDs into account") > > I still don't think the Fixes: tag here is valid (the code works as is > AFAICS), but I could drop it when applying the patch just fine. :-) > > That said, see below. > >> Cc: Sinan Kaya <okaya@codeaurora.org> >> Cc: Sakari Ailus <sakari.ailus@linux.intel.com> >> Cc: Vinod Koul <vinod.koul@intel.com> >> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> >> --- >> drivers/acpi/bus.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c >> index 58239abb0a72..761286e50067 100644 >> --- a/drivers/acpi/bus.c >> +++ b/drivers/acpi/bus.c >> @@ -769,7 +769,7 @@ static const struct acpi_device_id *__acpi_match_device( >> */ >> if (!strcmp(ACPI_DT_NAMESPACE_HID, hwid->id) >> && acpi_of_match_device(device, of_ids)) >> - return id; >> + return ERR_PTR(-ENODATA); > > Doesn't the comment above need to be updated? > > Also the return value here means "success", so why is an error the right choice? Moreover, if ids is NULL, then dereferencing it in the loop above won't work already, so if we get here, there is at least one entry in it. -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Feb 8, 2018 at 4:44 PM, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > On Thu, 2018-02-08 at 16:14 +0100, Rafael J. Wysocki wrote: >> On Wed, Feb 7, 2018 at 3:56 PM, Andy Shevchenko >> <andriy.shevchenko@linux.intel.com> wrote: >> > Instead of playing tricks with last invalid entry, >> > return simple -ENODATA error code cast to pointer. >> > >> > It would be good for future in case caller passes NULL pointer for >> > ID table. Moreover, caller can check the code to be sure what >> > happened >> > inside callee. >> > >> > Fixes: 2b9c698efa58 ("ACPI / scan: Take the PRP0001 position in the >> > list of IDs into account") >> >> I still don't think the Fixes: tag here is valid (the code works as is >> AFAICS), but I could drop it when applying the patch just fine. :-) > > OK. > >> > if (!strcmp(ACPI_DT_NAMESPACE_HID, hwid->id) >> > && acpi_of_match_device(device, of_ids)) >> > - return id; >> > + return ERR_PTR(-ENODATA); >> >> Doesn't the comment above need to be updated? > > Will do. > >> Also the return value here means "success", so why is an error the >> right choice? > > Because we need to return something which is not NULL. Naturally feels > the error code, esp. ENODATA, is quite suitable. We indeed have no data > in this case, and it's not a NULL case (not found / not match) — we have > a match. But this is an error code that means "success". May I call it rather confusing? > >> Overall, this really looks like a preparation for a future patch, so >> why not just say that straight away in the changelog? > > It's not _just_ a preparation, it mitigates the trick used in mentioned > by Fixes tag commit. > > I would rather update comment here, and add explanation to the commit > message to be sure it covers tricks mitigation and preparation purposes. This is not mitigation, sorry. It just replaces one possibly confusing thing with another. The code as is works as I said and this patch doesn't make it any better as far as I'm concerned. -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2018-02-08 at 16:48 +0100, Rafael J. Wysocki wrote: > On Thu, Feb 8, 2018 at 4:44 PM, Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: > > On Thu, 2018-02-08 at 16:14 +0100, Rafael J. Wysocki wrote: > > > > > > > > Also the return value here means "success", so why is an error the > > > right choice? > > > > Because we need to return something which is not NULL. Naturally > > feels > > the error code, esp. ENODATA, is quite suitable. We indeed have no > > data > > in this case, and it's not a NULL case (not found / not match) — we > > have > > a match. > > But this is an error code that means "success". May I call it rather > confusing? This function AFAICS does two things at once: - matches device against ID - returns matched ID entry in the table Return value combines those two into actually ternary option: - no match - match with ID - match without ID > > > Overall, this really looks like a preparation for a future patch, > > > so > > > why not just say that straight away in the changelog? > > > > It's not _just_ a preparation, it mitigates the trick used in > > mentioned > > by Fixes tag commit. > > > > I would rather update comment here, and add explanation to the > > commit > > message to be sure it covers tricks mitigation and preparation > > purposes. > > This is not mitigation, sorry. It just replaces one possibly > confusing thing with another. I would agree here... > The code as is works as I said and this patch doesn't make it any > better as far as I'm concerned. ...but not here. Instead of returning pointer to *something* (from caller point of view), we explicitly tell caller what of the above happened. We don't rely on the organization of ID table or its life time (though it's forever). I can say that is *slightly* better. But agree that is not cleanest solution I can come up with. I'm all ears on other possibilities how to get rid of that trick.
On Thu, Feb 8, 2018 at 4:59 PM, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > On Thu, 2018-02-08 at 16:48 +0100, Rafael J. Wysocki wrote: >> On Thu, Feb 8, 2018 at 4:44 PM, Andy Shevchenko >> <andriy.shevchenko@linux.intel.com> wrote: >> > On Thu, 2018-02-08 at 16:14 +0100, Rafael J. Wysocki wrote: >> > > > >> > >> > > Also the return value here means "success", so why is an error the >> > > right choice? >> > >> > Because we need to return something which is not NULL. Naturally >> > feels >> > the error code, esp. ENODATA, is quite suitable. We indeed have no >> > data >> > in this case, and it's not a NULL case (not found / not match) — we >> > have >> > a match. >> >> But this is an error code that means "success". May I call it rather >> confusing? > > This function AFAICS does two things at once: > - matches device against ID > - returns matched ID entry in the table > > Return value combines those two into actually ternary option: > - no match > - match with ID > - match without ID Right. And the caller knows when to expect the third case which is the whole point. >> > > Overall, this really looks like a preparation for a future patch, >> > > so >> > > why not just say that straight away in the changelog? >> > >> > It's not _just_ a preparation, it mitigates the trick used in >> > mentioned >> > by Fixes tag commit. >> > >> > I would rather update comment here, and add explanation to the >> > commit >> > message to be sure it covers tricks mitigation and preparation >> > purposes. >> >> This is not mitigation, sorry. It just replaces one possibly >> confusing thing with another. > > I would agree here... > >> The code as is works as I said and this patch doesn't make it any >> better as far as I'm concerned. > > ...but not here. Instead of returning pointer to *something* (from > caller point of view), we explicitly tell caller what of the above > happened. We don't rely on the organization of ID table or its life > time (though it's forever). > > I can say that is *slightly* better. But agree that is not cleanest > solution I can come up with. > > I'm all ears on other possibilities how to get rid of that trick. Please see my reply to the second patch, then. -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c index 58239abb0a72..761286e50067 100644 --- a/drivers/acpi/bus.c +++ b/drivers/acpi/bus.c @@ -769,7 +769,7 @@ static const struct acpi_device_id *__acpi_match_device( */ if (!strcmp(ACPI_DT_NAMESPACE_HID, hwid->id) && acpi_of_match_device(device, of_ids)) - return id; + return ERR_PTR(-ENODATA); } return NULL; }
Instead of playing tricks with last invalid entry, return simple -ENODATA error code cast to pointer. It would be good for future in case caller passes NULL pointer for ID table. Moreover, caller can check the code to be sure what happened inside callee. Fixes: 2b9c698efa58 ("ACPI / scan: Take the PRP0001 position in the list of IDs into account") Cc: Sinan Kaya <okaya@codeaurora.org> Cc: Sakari Ailus <sakari.ailus@linux.intel.com> Cc: Vinod Koul <vinod.koul@intel.com> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> --- drivers/acpi/bus.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)