diff mbox

[3/7] scsi: bfa: improve bfa_ioc_send_enable/disable data

Message ID 20171110153715.1929456-4-arnd@arndb.de (mailing list archive)
State Accepted
Headers show

Commit Message

Arnd Bergmann Nov. 10, 2017, 3:37 p.m. UTC
In bfa_ioc_send_enable, we use the deprecated do_gettimeofday() function
to read the current time. This is not a problem, since the firmware
interface is already limited to 32-bit timestamps, but it's better
to use ktime_get_seconds() and document what the limitation is.

I noticed that I did the same change in commit a5af83925363 ("bna:
avoid writing uninitialized data into hw registers") for the ethernet
driver. That commit also changed the "disable" funtion to initialize
the data we pass to the firmware properly, so I'm doing the same
thing here.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/scsi/bfa/bfa_ioc.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

Comments

Ben Hutchings Nov. 13, 2017, 2:08 p.m. UTC | #1
On Fri, 2017-11-10 at 16:37 +0100, Arnd Bergmann wrote:
> In bfa_ioc_send_enable, we use the deprecated do_gettimeofday() function
> to read the current time. This is not a problem, since the firmware
> interface is already limited to 32-bit timestamps, but it's better
> to use ktime_get_seconds() and document what the limitation is.
> 
> I noticed that I did the same change in commit a5af83925363 ("bna:
> avoid writing uninitialized data into hw registers") for the ethernet
> driver. That commit also changed the "disable" funtion to initialize
> the data we pass to the firmware properly, so I'm doing the same
> thing here.
>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>  drivers/scsi/bfa/bfa_ioc.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/scsi/bfa/bfa_ioc.c b/drivers/scsi/bfa/bfa_ioc.c
> index 256f4afaccf9..117332537763 100644
> --- a/drivers/scsi/bfa/bfa_ioc.c
> +++ b/drivers/scsi/bfa/bfa_ioc.c
> @@ -1809,13 +1809,12 @@ static void
>  bfa_ioc_send_enable(struct bfa_ioc_s *ioc)
>  {
>  	struct bfi_ioc_ctrl_req_s enable_req;
> -	struct timeval tv;
>  
>  	bfi_h2i_set(enable_req.mh, BFI_MC_IOC, BFI_IOC_H2I_ENABLE_REQ,
>  		    bfa_ioc_portid(ioc));
>  	enable_req.clscode = cpu_to_be16(ioc->clscode);
> -	do_gettimeofday(&tv);
> -	enable_req.tv_sec = be32_to_cpu(tv.tv_sec);
> +	/* unsigned 32-bit time_t overflow in y2106 */
> +	enable_req.tv_sec = be32_to_cpu(ktime_get_real_seconds());

The byte order conversion should also logically be cpu_to_be32(), not
be32_to_cpu().

Ben.

>  	bfa_ioc_mbox_send(ioc, &enable_req, sizeof(struct bfi_ioc_ctrl_req_s));
>  }
>  
> @@ -1826,6 +1825,9 @@ bfa_ioc_send_disable(struct bfa_ioc_s *ioc)
>  
>  	bfi_h2i_set(disable_req.mh, BFI_MC_IOC, BFI_IOC_H2I_DISABLE_REQ,
>  		    bfa_ioc_portid(ioc));
> +	disable_req.clscode = cpu_to_be16(ioc->clscode);
> +	/* unsigned 32-bit time_t overflow in y2106 */
> +	disable_req.tv_sec = be32_to_cpu(ktime_get_real_seconds());
>  	bfa_ioc_mbox_send(ioc, &disable_req, sizeof(struct bfi_ioc_ctrl_req_s));
>  }
>
Arnd Bergmann Nov. 13, 2017, 2:55 p.m. UTC | #2
On Mon, Nov 13, 2017 at 3:08 PM, Ben Hutchings
<ben.hutchings@codethink.co.uk> wrote:
> On Fri, 2017-11-10 at 16:37 +0100, Arnd Bergmann wrote:

>>
>>       bfi_h2i_set(enable_req.mh, BFI_MC_IOC, BFI_IOC_H2I_ENABLE_REQ,
>>                   bfa_ioc_portid(ioc));
>>       enable_req.clscode = cpu_to_be16(ioc->clscode);
>> -     do_gettimeofday(&tv);
>> -     enable_req.tv_sec = be32_to_cpu(tv.tv_sec);
>> +     /* unsigned 32-bit time_t overflow in y2106 */
>> +     enable_req.tv_sec = be32_to_cpu(ktime_get_real_seconds());
>
> The byte order conversion should also logically be cpu_to_be32(), not
> be32_to_cpu().
>

Right, I had not noticed that. I just tried fixing this, looking at what
sparse thinks of the file as a whole. I found it basically hopeless
to fix the endianess warnings in this driver, it seems completely
random here whether they are used correctly, in the opposite
direction as expected, or just flip data in place as in

attr->part[i].part_off = be32_to_cpu(f->part[i].part_off);

I'd rather not touch that part. IIRC I had at some point spent
a day trying to clean up the "warning: symbol
 'bfa_flash_sem_get' was not declared. Should it be static?"
sparse warnings for some driver, before giving up for
similar reasons.

       Arnd
diff mbox

Patch

diff --git a/drivers/scsi/bfa/bfa_ioc.c b/drivers/scsi/bfa/bfa_ioc.c
index 256f4afaccf9..117332537763 100644
--- a/drivers/scsi/bfa/bfa_ioc.c
+++ b/drivers/scsi/bfa/bfa_ioc.c
@@ -1809,13 +1809,12 @@  static void
 bfa_ioc_send_enable(struct bfa_ioc_s *ioc)
 {
 	struct bfi_ioc_ctrl_req_s enable_req;
-	struct timeval tv;
 
 	bfi_h2i_set(enable_req.mh, BFI_MC_IOC, BFI_IOC_H2I_ENABLE_REQ,
 		    bfa_ioc_portid(ioc));
 	enable_req.clscode = cpu_to_be16(ioc->clscode);
-	do_gettimeofday(&tv);
-	enable_req.tv_sec = be32_to_cpu(tv.tv_sec);
+	/* unsigned 32-bit time_t overflow in y2106 */
+	enable_req.tv_sec = be32_to_cpu(ktime_get_real_seconds());
 	bfa_ioc_mbox_send(ioc, &enable_req, sizeof(struct bfi_ioc_ctrl_req_s));
 }
 
@@ -1826,6 +1825,9 @@  bfa_ioc_send_disable(struct bfa_ioc_s *ioc)
 
 	bfi_h2i_set(disable_req.mh, BFI_MC_IOC, BFI_IOC_H2I_DISABLE_REQ,
 		    bfa_ioc_portid(ioc));
+	disable_req.clscode = cpu_to_be16(ioc->clscode);
+	/* unsigned 32-bit time_t overflow in y2106 */
+	disable_req.tv_sec = be32_to_cpu(ktime_get_real_seconds());
 	bfa_ioc_mbox_send(ioc, &disable_req, sizeof(struct bfi_ioc_ctrl_req_s));
 }