* [PATCH v2 0/2] Fix sloppy 'timeout' parameter type in libata-core.c
@ 2022-06-15 19:38 Sergey Shtylyov
2022-06-15 19:38 ` [PATCH v2 1/2] ata: libata-core: make ata_exec_internal_sg() *static* Sergey Shtylyov
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: Sergey Shtylyov @ 2022-06-15 19:38 UTC (permalink / raw)
To: Damien Le Moal, linux-ide
The libata code too often uses the *unsigned long* type for the millisecond
timeouts, while the kernel functions like msecs_to_jiffies() or msleep() only
take *unsigned int* parameters for those. Start fixing the timeout types from
ata_exec_internal[_sg]() that triggered the SVACE static analyzer; there's
going to be a large continuation series somewhat later...
Sergey Shtylyov (2):
ata: libata-core: make ata_exec_internal_sg() *static*
ata: libata-core: fix sloppy parameter type in ata_exec_internal[_sg]()
drivers/ata/libata-core.c | 12 ++++++------
drivers/ata/libata.h | 6 +-----
2 files changed, 7 insertions(+), 11 deletions(-)
--
2.26.3
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v2 1/2] ata: libata-core: make ata_exec_internal_sg() *static*
2022-06-15 19:38 [PATCH v2 0/2] Fix sloppy 'timeout' parameter type in libata-core.c Sergey Shtylyov
@ 2022-06-15 19:38 ` Sergey Shtylyov
2022-06-17 7:48 ` Damien Le Moal
2022-06-15 19:38 ` [PATCH v2 2/2] ata: libata-core: fix sloppy parameter type in ata_exec_internal[_sg]() Sergey Shtylyov
2022-06-15 19:49 ` [PATCH v2 0/2] Fix sloppy 'timeout' parameter type in libata-core.c Sergey Shtylyov
2 siblings, 1 reply; 10+ messages in thread
From: Sergey Shtylyov @ 2022-06-15 19:38 UTC (permalink / raw)
To: Damien Le Moal, linux-ide
ata_exec_internal_sg() is only called by ata_exec_internal() further in
the same file, so we can make it *static* and remove its prototype from
drivers/ata/libata.h...
Suggested-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
Signed-off-by: Sergey Shtylyov <s.shtylyov@omp.ru>
---
Changes in version 2:
- new patch.
drivers/ata/libata-core.c | 8 ++++----
drivers/ata/libata.h | 4 ----
2 files changed, 4 insertions(+), 8 deletions(-)
diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 980328a4b896..3cc1312a2622 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -1467,10 +1467,10 @@ static void ata_qc_complete_internal(struct ata_queued_cmd *qc)
* RETURNS:
* Zero on success, AC_ERR_* mask on failure
*/
-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)
+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)
{
struct ata_link *link = dev->link;
struct ata_port *ap = link->ap;
diff --git a/drivers/ata/libata.h b/drivers/ata/libata.h
index 926a7f41303d..1446a482835d 100644
--- a/drivers/ata/libata.h
+++ b/drivers/ata/libata.h
@@ -53,10 +53,6 @@ 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);
-extern unsigned ata_exec_internal_sg(struct ata_device *dev,
- struct ata_taskfile *tf, const u8 *cdb,
- int dma_dir, struct scatterlist *sg,
- unsigned int n_elem, unsigned long 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,
--
2.26.3
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH v2 2/2] ata: libata-core: fix sloppy parameter type in ata_exec_internal[_sg]()
2022-06-15 19:38 [PATCH v2 0/2] Fix sloppy 'timeout' parameter type in libata-core.c Sergey Shtylyov
2022-06-15 19:38 ` [PATCH v2 1/2] ata: libata-core: make ata_exec_internal_sg() *static* Sergey Shtylyov
@ 2022-06-15 19:38 ` Sergey Shtylyov
2022-06-17 7:44 ` Damien Le Moal
2022-06-19 23:23 ` Damien Le Moal
2022-06-15 19:49 ` [PATCH v2 0/2] Fix sloppy 'timeout' parameter type in libata-core.c Sergey Shtylyov
2 siblings, 2 replies; 10+ messages in thread
From: Sergey Shtylyov @ 2022-06-15 19:38 UTC (permalink / raw)
To: Damien Le Moal, linux-ide
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...
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,
--
2.26.3
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v2 0/2] Fix sloppy 'timeout' parameter type in libata-core.c
2022-06-15 19:38 [PATCH v2 0/2] Fix sloppy 'timeout' parameter type in libata-core.c Sergey Shtylyov
2022-06-15 19:38 ` [PATCH v2 1/2] ata: libata-core: make ata_exec_internal_sg() *static* Sergey Shtylyov
2022-06-15 19:38 ` [PATCH v2 2/2] ata: libata-core: fix sloppy parameter type in ata_exec_internal[_sg]() Sergey Shtylyov
@ 2022-06-15 19:49 ` Sergey Shtylyov
2 siblings, 0 replies; 10+ messages in thread
From: Sergey Shtylyov @ 2022-06-15 19:49 UTC (permalink / raw)
To: Damien Le Moal, linux-ide
On 6/15/22 10:38 PM, Sergey Shtylyov wrote:
> The libata code too often uses the *unsigned long* type for the millisecond
> timeouts, while the kernel functions like msecs_to_jiffies() or msleep() only
> take *unsigned int* parameters for those. Start fixing the timeout types from
> ata_exec_internal[_sg]() that triggered the SVACE static analyzer; there's
> going to be a large continuation series somewhat later...
>
> Sergey Shtylyov (2):
> ata: libata-core: make ata_exec_internal_sg() *static*
> ata: libata-core: fix sloppy parameter type in ata_exec_internal[_sg]()
>
> drivers/ata/libata-core.c | 12 ++++++------
> drivers/ata/libata.h | 6 +-----
> 2 files changed, 7 insertions(+), 11 deletions(-)
Forgot to mention the patches are against the 'for-next' branch of Damien's
'libata.git' repo... :-/
MBR, Sergey
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 2/2] ata: libata-core: fix sloppy parameter type in ata_exec_internal[_sg]()
2022-06-15 19:38 ` [PATCH v2 2/2] ata: libata-core: fix sloppy parameter type in ata_exec_internal[_sg]() Sergey Shtylyov
@ 2022-06-17 7:44 ` Damien Le Moal
2022-06-17 17:27 ` Sergey Shtylyov
2022-06-19 23:23 ` Damien Le Moal
1 sibling, 1 reply; 10+ messages in thread
From: Damien Le Moal @ 2022-06-17 7:44 UTC (permalink / raw)
To: Sergey Shtylyov, linux-ide
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 1/2] ata: libata-core: make ata_exec_internal_sg() *static*
2022-06-15 19:38 ` [PATCH v2 1/2] ata: libata-core: make ata_exec_internal_sg() *static* Sergey Shtylyov
@ 2022-06-17 7:48 ` Damien Le Moal
0 siblings, 0 replies; 10+ messages in thread
From: Damien Le Moal @ 2022-06-17 7:48 UTC (permalink / raw)
To: Sergey Shtylyov, linux-ide
On 6/16/22 04:38, Sergey Shtylyov wrote:
> ata_exec_internal_sg() is only called by ata_exec_internal() further in
> the same file, so we can make it *static* and remove its prototype from
> drivers/ata/libata.h...
>
> Suggested-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
> Signed-off-by: Sergey Shtylyov <s.shtylyov@omp.ru>
> ---
> Changes in version 2:
> - new patch.
>
> drivers/ata/libata-core.c | 8 ++++----
> drivers/ata/libata.h | 4 ----
> 2 files changed, 4 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index 980328a4b896..3cc1312a2622 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -1467,10 +1467,10 @@ static void ata_qc_complete_internal(struct ata_queued_cmd *qc)
> * RETURNS:
> * Zero on success, AC_ERR_* mask on failure
> */
> -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)
> +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)
> {
> struct ata_link *link = dev->link;
> struct ata_port *ap = link->ap;
> diff --git a/drivers/ata/libata.h b/drivers/ata/libata.h
> index 926a7f41303d..1446a482835d 100644
> --- a/drivers/ata/libata.h
> +++ b/drivers/ata/libata.h
> @@ -53,10 +53,6 @@ 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);
> -extern unsigned ata_exec_internal_sg(struct ata_device *dev,
> - struct ata_taskfile *tf, const u8 *cdb,
> - int dma_dir, struct scatterlist *sg,
> - unsigned int n_elem, unsigned long 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,
Applied this one only to for-5.20. Thanks !
--
Damien Le Moal
Western Digital Research
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 2/2] ata: libata-core: fix sloppy parameter type in ata_exec_internal[_sg]()
2022-06-17 7:44 ` Damien Le Moal
@ 2022-06-17 17:27 ` Sergey Shtylyov
2022-06-18 6:45 ` Damien Le Moal
0 siblings, 1 reply; 10+ messages in thread
From: Sergey Shtylyov @ 2022-06-17 17:27 UTC (permalink / raw)
To: Damien Le Moal, linux-ide
Hello!
On 6/17/22 10:44 AM, Damien Le Moal 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.
I know. All timeouts stored in those tables fit into *unsigned int* (except
the last ones which get truncated from ULONG_MAX to UINT_MAX anyways).
Note that *unsigned long* has always been silently truncated to *unsigned int*
when calling msecs_to_jiffies() in ata_exec_internal_sg(); nothing changes with
my patch, it just gets truncated a bit earlier...
> 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()).
It shouldn't matter whether we do it before or after this patch, due to
truncation which happens even now (I have such a patch, but no description
yet). I was planning to address that as a part of the large patch series,
most probably next week (and I wrote about that in the cover letter)...
> Hmm ? Thoughts ?
Was I clear enough? :-)
>> Found by Linux Verification Center (linuxtesting.org) with the SVACE static
>> analysis tool.
I wanted to fix SVACE's reports 1st, then the rest of the sloppy timeout
typing...
>>
>> Signed-off-by: Sergey Shtylyov <s.shtylyov@omp.ru>
[...]
MBR, Sergey
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 2/2] ata: libata-core: fix sloppy parameter type in ata_exec_internal[_sg]()
2022-06-17 17:27 ` Sergey Shtylyov
@ 2022-06-18 6:45 ` Damien Le Moal
2022-06-18 19:49 ` Sergey Shtylyov
0 siblings, 1 reply; 10+ messages in thread
From: Damien Le Moal @ 2022-06-18 6:45 UTC (permalink / raw)
To: Sergey Shtylyov, linux-ide
On 6/18/22 02:27, Sergey Shtylyov wrote:
> Hello!
>
> On 6/17/22 10:44 AM, Damien Le Moal 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.
>
> I know. All timeouts stored in those tables fit into *unsigned int* (except
> the last ones which get truncated from ULONG_MAX to UINT_MAX anyways).
> Note that *unsigned long* has always been silently truncated to *unsigned int*
> when calling msecs_to_jiffies() in ata_exec_internal_sg(); nothing changes with
> my patch, it just gets truncated a bit earlier...
>
>> 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()).
>
> It shouldn't matter whether we do it before or after this patch, due to
> truncation which happens even now (I have such a patch, but no description
> yet). I was planning to address that as a part of the large patch series,
> most probably next week (and I wrote about that in the cover letter)...
>
>> Hmm ? Thoughts ?
>
> Was I clear enough? :-)
>
>>> Found by Linux Verification Center (linuxtesting.org) with the SVACE static
>>> analysis tool.
>
> I wanted to fix SVACE's reports 1st, then the rest of the sloppy timeout
> typing...
Fine. But in the spirit of fixing everything with these types, please add
a patch to also convert the timeout tables to unsigned int. It is never a
good idea to have code silently truncate long to int as that can be a pain
to debug. So let's go all the way and fix this.
If you cannot do the additional patch, I will do it.
>
>>>
>>> Signed-off-by: Sergey Shtylyov <s.shtylyov@omp.ru>
> [...]
>
> MBR, Sergey
--
Damien Le Moal
Western Digital Research
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 2/2] ata: libata-core: fix sloppy parameter type in ata_exec_internal[_sg]()
2022-06-18 6:45 ` Damien Le Moal
@ 2022-06-18 19:49 ` Sergey Shtylyov
0 siblings, 0 replies; 10+ messages in thread
From: Sergey Shtylyov @ 2022-06-18 19:49 UTC (permalink / raw)
To: Damien Le Moal, linux-ide
Hello!
On 6/18/22 9:45 AM, Damien Le Moal 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.
>>
>> I know. All timeouts stored in those tables fit into *unsigned int* (except
>> the last ones which get truncated from ULONG_MAX to UINT_MAX anyways).
>> Note that *unsigned long* has always been silently truncated to *unsigned int*
>> when calling msecs_to_jiffies() in ata_exec_internal_sg(); nothing changes with
>> my patch, it just gets truncated a bit earlier...
>>
>>> 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()).
>>
>> It shouldn't matter whether we do it before or after this patch, due to
>> truncation which happens even now (I have such a patch, but no description
>> yet). I was planning to address that as a part of the large patch series,
>> most probably next week (and I wrote about that in the cover letter)...
>>
>>> Hmm ? Thoughts ?
>>
>> Was I clear enough? :-)
>>
>>>> Found by Linux Verification Center (linuxtesting.org) with the SVACE static
>>>> analysis tool.
>>
>> I wanted to fix SVACE's reports 1st, then the rest of the sloppy timeout
>> typing...
>
> Fine. But in the spirit of fixing everything with these types, please add
> a patch to also convert the timeout tables to unsigned int. It is never a
> good idea to have code silently truncate long to int
I even wonder why C allows that... and the static analyzers don't seem to
complain about that too...
> as that can be a pain
> to debug. So let's go all the way and fix this.
OK.
> If you cannot do the additional patch, I will do it.
I can. I have the patch already, just without the description yet...
>>>> Signed-off-by: Sergey Shtylyov <s.shtylyov@omp.ru>
>> [...]
MBR, Sergey
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 2/2] ata: libata-core: fix sloppy parameter type in ata_exec_internal[_sg]()
2022-06-15 19:38 ` [PATCH v2 2/2] ata: libata-core: fix sloppy parameter type in ata_exec_internal[_sg]() Sergey Shtylyov
2022-06-17 7:44 ` Damien Le Moal
@ 2022-06-19 23:23 ` Damien Le Moal
1 sibling, 0 replies; 10+ messages in thread
From: Damien Le Moal @ 2022-06-19 23:23 UTC (permalink / raw)
To: Sergey Shtylyov, linux-ide
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...
>
> Found by Linux Verification Center (linuxtesting.org) with the SVACE static
> analysis tool.
>
> Signed-off-by: Sergey Shtylyov <s.shtylyov@omp.ru>
Applied to for-5.20. Thanks.
--
Damien Le Moal
Western Digital Research
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2022-06-19 23:23 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-06-15 19:38 [PATCH v2 0/2] Fix sloppy 'timeout' parameter type in libata-core.c Sergey Shtylyov
2022-06-15 19:38 ` [PATCH v2 1/2] ata: libata-core: make ata_exec_internal_sg() *static* Sergey Shtylyov
2022-06-17 7:48 ` Damien Le Moal
2022-06-15 19:38 ` [PATCH v2 2/2] ata: libata-core: fix sloppy parameter type in ata_exec_internal[_sg]() Sergey Shtylyov
2022-06-17 7:44 ` Damien Le Moal
2022-06-17 17:27 ` Sergey Shtylyov
2022-06-18 6:45 ` Damien Le Moal
2022-06-18 19:49 ` Sergey Shtylyov
2022-06-19 23:23 ` Damien Le Moal
2022-06-15 19:49 ` [PATCH v2 0/2] Fix sloppy 'timeout' parameter type in libata-core.c Sergey Shtylyov
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).