[PATCH v1] drm/mipi_dbi: Use simple right shift instead of double negation

[Sean Paul]

On Thu, Oct 17, 2019 at 05:00:54PM +0300, Andy Shevchenko wrote:
> On Thu, Oct 17, 2019 at 09:10:52AM -0400, Sean Paul wrote:
> > On Thu, Oct 17, 2019 at 02:49:12PM +0300, Andy Shevchenko wrote:
> > > GCC complains about dubious bitwise OR operand:
> > > 
> > > drivers/gpu/drm/drm_mipi_dbi.c:1024:49: warning: dubious: x | !y
> > >   CC [M]  drivers/gpu/drm/drm_mipi_dbi.o
> > > 
> > > As long as buffer is consist of byte (u8) values, we may use
> > > simple right shift and satisfy compiler. It also reduces amount of
> > > operations needed.
> > > 
> > > Signed-off-by: Andy Shevchenko <andriy.shevchenko at linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/drm_mipi_dbi.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_mipi_dbi.c b/drivers/gpu/drm/drm_mipi_dbi.c
> > > index 1961f713aaab..445e88b1fc9a 100644
> > > --- a/drivers/gpu/drm/drm_mipi_dbi.c
> > > +++ b/drivers/gpu/drm/drm_mipi_dbi.c
> > > @@ -1021,7 +1021,7 @@ static int mipi_dbi_typec3_command_read(struct mipi_dbi *dbi, u8 *cmd,
> > >  		unsigned int i;
> > >  
> > >  		for (i = 0; i < len; i++)
> > > -			data[i] = (buf[i] << 1) | !!(buf[i + 1] & BIT(7));
> > > +			data[i] = (buf[i] << 1) | (buf[i + 1] >> 7);
> > 
> > You should probably have ((buf[i + 1] >> 7) & 0x1) to be super safe.
> 
> This is superfluous as long as buf is declared as u8 (see commit message).
> 

Yeah, I saw that, hence the "super safe" comment. My point is that writing that
disclaimer in the commit message doesn't actually protect the operation if the
type changes, while masking off the first bit does.

> > Do you know anything about this code? It seems like nothing is protecting us
> > from overrunning buf in this loop. We're just assuming that len < tr[1].len
> > through this loop and I'm not sure what's protecting us from looking where we
> > shouldn't.
> 
> It I'm not mistaken this is the case, we have it strong less than transfer
> length.
> 

Thanks, I looked at the code and you're right. The possible values of tr[1].len are
len or len + 1.

Sean

> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
> 

-- 
Sean Paul, Software Engineer, Google / Chromium OS