Re: [PATCH v2 2/2] ata: libata-core: fix sloppy parameter type in ata_exec_internal[_sg]()

[Damien Le Moal <damien.lemoal@opensource.wdc.com>]

On 6/16/22 04:38, Sergey Shtylyov wrote:
> Make the 'timeout' parameter to ata_exec_internal_sg() *unsigned int* as
> msecs_to_jiffies() that it calls takes just *unsigned int* for the time in
> milliseconds. Then follow the suit with ata_exec_internal(), its only
> caller; also fix up ata_dev_set_feature(), the only ata_exec_internal()'s
> caller  that explicitly passes *unsigned long* variable for timeout...

Checking this, struct ata_eh_cmd_timeout_ent uses an unsigned long timeout
and ata_internal_cmd_timeout() returns an unsigned long which is stored
into the unsigned int timeout variable. So it may be good to add another
prep patch before this one to cleanup the auto_timeout stuff (struct
ata_eh_cmd_timeout_ent and ata_internal_cmd_timeout()).

Hmm ? Thoughts ?

> 
> Found by Linux Verification Center (linuxtesting.org) with the SVACE static
> analysis tool.
> 
> Signed-off-by: Sergey Shtylyov <s.shtylyov@omp.ru>
> ---
> Changes in version 2:
> - rebased atop of the new patch #1.
> 
>  drivers/ata/libata-core.c | 6 +++---
>  drivers/ata/libata.h      | 2 +-
>  2 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index 3cc1312a2622..03a08d1e666a 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -1470,7 +1470,7 @@ static void ata_qc_complete_internal(struct ata_queued_cmd *qc)
>  static unsigned ata_exec_internal_sg(struct ata_device *dev,
>  				     struct ata_taskfile *tf, const u8 *cdb,
>  				     int dma_dir, struct scatterlist *sgl,
> -				     unsigned int n_elem, unsigned long timeout)
> +				     unsigned int n_elem, unsigned int timeout)
>  {
>  	struct ata_link *link = dev->link;
>  	struct ata_port *ap = link->ap;
> @@ -1645,7 +1645,7 @@ static unsigned ata_exec_internal_sg(struct ata_device *dev,
>  unsigned ata_exec_internal(struct ata_device *dev,
>  			   struct ata_taskfile *tf, const u8 *cdb,
>  			   int dma_dir, void *buf, unsigned int buflen,
> -			   unsigned long timeout)
> +			   unsigned int timeout)
>  {
>  	struct scatterlist *psg = NULL, sg;
>  	unsigned int n_elem = 0;
> @@ -4342,7 +4342,7 @@ unsigned int ata_dev_set_feature(struct ata_device *dev, u8 enable, u8 feature)
>  {
>  	struct ata_taskfile tf;
>  	unsigned int err_mask;
> -	unsigned long timeout = 0;
> +	unsigned int timeout = 0;
>  
>  	/* set up set-features taskfile */
>  	ata_dev_dbg(dev, "set features - SATA features\n");
> diff --git a/drivers/ata/libata.h b/drivers/ata/libata.h
> index 1446a482835d..8292d4cdc22b 100644
> --- a/drivers/ata/libata.h
> +++ b/drivers/ata/libata.h
> @@ -52,7 +52,7 @@ extern u64 ata_tf_read_block(const struct ata_taskfile *tf,
>  extern unsigned ata_exec_internal(struct ata_device *dev,
>  				  struct ata_taskfile *tf, const u8 *cdb,
>  				  int dma_dir, void *buf, unsigned int buflen,
> -				  unsigned long timeout);
> +				  unsigned int timeout);
>  extern int ata_wait_ready(struct ata_link *link, unsigned long deadline,
>  			  int (*check_ready)(struct ata_link *link));
>  extern int ata_dev_read_id(struct ata_device *dev, unsigned int *p_class,


-- 
Damien Le Moal
Western Digital Research