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

[Sergei Shtylyov <sergei.shtylyov@gmail.com>]

Hello!

   Replying from my Gmail address, the corporate IMAP server is still AWOL... :-(
   Not sure what happened to the subject: "[_sg]" somehow got moved before "fix"...

On 6/15/22 2:46 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...
>>>>
>>>> Found by Linux Verification Center (linuxtesting.org) with the SVACE static
>>>> analysis tool.
>>>
>>> Since you are changing this function signature, can you also make it
>>> static since it is only used in libata-core.c ? The declaration in
>>> drivers/ata/libata.h is useless.
>>
>>    Hopefully you don't mean I should do it in the same patch? :-)
> 
> The patch you sent is changing the function signature.

   Actually, it changes 2 function signatures...

> So yes, doing it in the same patch is fine with me.

   But not with me. Adding a "drove by" signature change doesn't go together
well with the "found by SVACE" note, to start with...
   Moreover, I've created 2 patches already, with the 1st patch making
ata_exec_internal_sg() *static*...

> I do not see the need for 2 patches for that.

   This patch as is already does enough to violate the "do one thing per
patch" rule, to add even more violations. :-)

> Instead of "fix sloppy parameter type in ata_exec_internal_sg",
> rename the patch "fix ata_exec_internal_sg signature" and then all changes
> naturally belong to the same patch.

   Thanks, no -- I'd like to keep the emphasis on "sloppy parameter type".

MBR, Sergey