On Thu, Feb 8, 2018 at 4:44 PM, Andy Shevchenko
<andriy.shevchenko@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx> 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 dmaengine" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
