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@xxxxxx>
>
> ---
> 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)
--
Damien Le Moal
Western Digital Research
