libata: ata_{sff|std}_prereset() always return 0

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@omp.ru>

---
This patch is against the 'for-next' branch of Damien Le Moal's 'libata.git' repo.

 drivers/ata/libata-core.c |    2 +-
 drivers/ata/libata-sff.c  |    6 ++----
 2 files changed, 3 insertions(+), 5 deletions(-)

On 1/29/22 05:45, Sergey Shtylyov 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@omp.ru>
> 
> ---
> This patch is against the 'for-next' branch of Damien Le Moal's 'libata.git' repo.
> 
>  drivers/ata/libata-core.c |    2 +-
>  drivers/ata/libata-sff.c  |    6 ++----
>  2 files changed, 3 insertions(+), 5 deletions(-)
> 
> Index: libata/drivers/ata/libata-core.c
> ===================================================================
> --- libata.orig/drivers/ata/libata-core.c
> +++ libata/drivers/ata/libata-core.c
> @@ -3568,7 +3568,7 @@ EXPORT_SYMBOL_GPL(ata_wait_after_reset);
>   *	Kernel thread context (may sleep)
>   *
>   *	RETURNS:
> - *	0 on success, -errno otherwise.
> + *	0 on success.
>   */
>  int ata_std_prereset(struct ata_link *link, unsigned long deadline)
>  {
> 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.

>   */
>  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.
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.
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. 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 ?

>  
>  	/* if we're about to do hardreset, nothing more to do */
>  	if (ehc->i.action & ATA_EH_HARDRESET)

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@omp.ru>

[...]
>> 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