Re: [PATCH v2] libata: ata_{sff|std}_prereset() always return 0

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 2/2/22 05:51, 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@xxxxxx>
> 
> ---
> This patch is against the 'for-next' branch of Damien Le Moal's 'libata.git'
> repo.
> 
> Changes in version 2:
> - fixed up the 'kernel-doc' comments on the function results.
> 
>  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.
> + *	Always 0.
>   */
>  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.
> + *	Always 0.
>   */
>  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);

Not a fan of removing the check here unless ata_std_prereset() is turned
into a void function. But doing that needs more changes with little
value. So at least can you add a comment above this call ? Something like:

	/* Standard prereset is best-effort and always return 0 */
	ata_std_prereset(link, deadline);

So that it is clear that we are aware that the function is non-void ?

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


-- 
Damien Le Moal
Western Digital Research



[Index of Archives]     [Linux Filesystems]     [Linux SCSI]     [Linux RAID]     [Git]     [Kernel Newbies]     [Linux Newbie]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Samba]     [Device Mapper]

  Powered by Linux