On 1/29/22 8:24 AM, Damien Le Moal wrote:
>> ata_std_prereset() always returns 0, hence the check in ata_sff_prereset()
>> is pointless and thus it also can return only 0 (however, we cannot change
>> the prototypes of ata_{sff|std}_prereset() as they implement the driver's
>> prereset() method).
>>
>> Found by Linux Verification Center (linuxtesting.org) with the SVACE static
>> analysis tool.
>>
>> Signed-off-by: Sergey Shtylyov <s.shtylyov@xxxxxx>
[...]
>> Index: libata/drivers/ata/libata-sff.c
>> ===================================================================
>> --- libata.orig/drivers/ata/libata-sff.c
>> +++ libata/drivers/ata/libata-sff.c
>> @@ -1708,16 +1708,14 @@ EXPORT_SYMBOL_GPL(ata_sff_thaw);
>> * Kernel thread context (may sleep)
>> *
>> * RETURNS:
>> - * 0 on success, -errno otherwise.
>> + * 0 on success.
>
> Well, since the function *always* return 0, I would just say so, since
> "on success" can of imply that there may be failures.
> Something like "0, always" would be better I think.
Agreed.
>> */
>> int ata_sff_prereset(struct ata_link *link, unsigned long deadline)
>> {
>> struct ata_eh_context *ehc = &link->eh_context;
>> int rc;
>>
>> - rc = ata_std_prereset(link, deadline);
>> - if (rc)
>> - return rc;
>> + ata_std_prereset(link, deadline);
>
> I am not a big fan of such change. If ata_std_prereset is changed to
> actually return an error, the above change would result in a bug...
>
> What about fixing things properly:
> 1) The ata_std_prereset comment says it: ata_std_prereset should not
> fail, so let's make it a void function.
Hm... we can't, as ata_std_prereset() sometimes directly implement prereset()
and pmp_prereset() methods and those should return *int*...
> 2) The only direct user of ata_std_prereset as the pmp_prereset port op
> is libata-pmp.c, so let's just add a static function there that calls
> ata_std_prereset and return 0.
I think you've missed 'ata_base_port_ops' also using ata_std_prereset()...
> 3) ata_sff_prereset also always return 0. So ideally, we should also
> make it void and have a wrapper for the drivers using it as the prereset
> port op.
There are no drivers using it as a method -- only libata-sff.c does it,
others have a wrapper around it.
> But there are more places to change than for ata_std_prereset,
> so not sure this makes the code cleaner. Worth looking into it though.
>
> Thoughts ?
I think I'd like to keep my approach...
[...]
MBR, Sergey
