[Fwd: Re: [PATCH 1/1] drivers/scsi/tmscsim.c: replace shift loop by ilog2]

[Fwd: Re: [PATCH 1/1] drivers/scsi/tmscsim.c: replace shift loop by ilog2]

[Joe Perches]

Hello Kurt.

If you're still interested in tmscim.

Your garloff@suse.de address bounces....

btw:  Your MAINTAINERS entry is out of date.

MAINTAINERS-DC390/AM53C974 SCSI driver
MAINTAINERS:M:  Kurt Garloff <garloff@suse.de>
MAINTAINERS:W:  http://www.garloff.de/kurt/linux/dc390/
MAINTAINERS-M:  Guennadi Liakhovetski <g.liakhovetski@gmx.de>
MAINTAINERS-S:  Maintained
MAINTAINERS-F:  drivers/scsi/tmscsim.*

-------- Forwarded Message --------
From: Joe Perches <joe@perches.com>
To: Fabian Frederick <fabf@skynet.be>
Cc: linux-kernel <linux-kernel@vger.kernel.org>, Kurt Garloff
<garloff@suse.de>, akpm <akpm@linux-foundation.org>
Subject: Re: [PATCH 1/1] drivers/scsi/tmscsim.c: replace shift loop by
ilog2

On Tue, 2014-05-20 at 18:48 +0200, Fabian Frederick wrote:
> Cc: Kurt Garloff <garloff@suse.de>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Signed-off-by: Fabian Frederick <fabf@skynet.be>
> ---
>  drivers/scsi/tmscsim.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/scsi/tmscsim.c b/drivers/scsi/tmscsim.c
[]
> @@ -1611,7 +1611,7 @@ dc390_Reselect( struct dc390_acb* pACB )
>        printk (KERN_ERR "DC390: Reselection must select host adapter: %02x!\n", lun);
>      else
>        lun ^= 1 << pACB->pScsiHost->this_id; /* Mask AdapterID */
> -    id = 0; while (lun >>= 1) id++;
> +    id = ilog2(lun);
>      /* Get LUN */
>      lun = DC390_read8 (ScsiFifo);
>      if (!(lun & IDENTIFY_BASE)) printk (KERN_ERR "DC390: Resel: Expect identify message!\n");

Hey Fabian.

You've submitted several of these now.

Have you gone through all of these to make sure
that the ilog2(<foo>) uses are always using a
non-zero <foo>?

If so, that should be mentioned in the changelog.

These should probably add #include <linux/log2.h>

It would have been nicer to use a patch series
and a cover letter so I could make this reply to
to cover letter.

Please take the time to setup and use git send-email
instead of sylpheed.

Re: [Fwd: Re: [PATCH 1/1] drivers/scsi/tmscsim.c: replace shift loop by ilog2]

[Guennadi Liakhovetski]

Hi

On Tue, 20 May 2014, Joe Perches wrote:

> Hello Kurt.
> 
> If you're still interested in tmscim.
> 
> Your garloff@suse.de address bounces....
> 
> btw:  Your MAINTAINERS entry is out of date.
> 
> MAINTAINERS-DC390/AM53C974 SCSI driver
> MAINTAINERS:M:  Kurt Garloff <garloff@suse.de>
> MAINTAINERS:W:  http://www.garloff.de/kurt/linux/dc390/
> MAINTAINERS-M:  Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> MAINTAINERS-S:  Maintained
> MAINTAINERS-F:  drivers/scsi/tmscsim.*
> 
> -------- Forwarded Message --------
> From: Joe Perches <joe@perches.com>
> To: Fabian Frederick <fabf@skynet.be>
> Cc: linux-kernel <linux-kernel@vger.kernel.org>, Kurt Garloff
> <garloff@suse.de>, akpm <akpm@linux-foundation.org>
> Subject: Re: [PATCH 1/1] drivers/scsi/tmscsim.c: replace shift loop by
> ilog2
> 
> On Tue, 2014-05-20 at 18:48 +0200, Fabian Frederick wrote:
> > Cc: Kurt Garloff <garloff@suse.de>
> > Cc: Andrew Morton <akpm@linux-foundation.org>
> > Signed-off-by: Fabian Frederick <fabf@skynet.be>
> > ---
> >  drivers/scsi/tmscsim.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/scsi/tmscsim.c b/drivers/scsi/tmscsim.c
> []
> > @@ -1611,7 +1611,7 @@ dc390_Reselect( struct dc390_acb* pACB )
> >        printk (KERN_ERR "DC390: Reselection must select host adapter: %02x!\n", lun);
> >      else
> >        lun ^= 1 << pACB->pScsiHost->this_id; /* Mask AdapterID */
> > -    id = 0; while (lun >>= 1) id++;
> > +    id = ilog2(lun);
> >      /* Get LUN */
> >      lun = DC390_read8 (ScsiFifo);
> >      if (!(lun & IDENTIFY_BASE)) printk (KERN_ERR "DC390: Resel: Expect identify message!\n");
> 
> Hey Fabian.
> 
> You've submitted several of these now.
> 
> Have you gone through all of these to make sure
> that the ilog2(<foo>) uses are always using a
> non-zero <foo>?

I think here at least, lun indeed can be 0, at least there's a test there:

    if (!(lun & (1 << pACB->pScsiHost->this_id)))
      printk (KERN_ERR "DC390: Reselection must select host adapter: %02x!\n", lun);

So, it is possible, that the "1 << this_id" bit is not set, then possibly 
it can be 0 too... Although from the hardware / SCSI PoV I'm not sure TBH. 
To be on the safe side we could at least add a check for 0 and only use 
ilog2, for != 0.

Thanks
Guennadi

> If so, that should be mentioned in the changelog.
> 
> These should probably add #include <linux/log2.h>
> 
> It would have been nicer to use a patch series
> and a cover letter so I could make this reply to
> to cover letter.
> 
> Please take the time to setup and use git send-email
> instead of sylpheed.
> 
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/