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

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

[Sean Paul]

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@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.

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.

Sean

>  	}
>  
>  	MIPI_DBI_DEBUG_COMMAND(*cmd, data, len);
> -- 
> 2.23.0
> 

-- 
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [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@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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Noralf Trønnes]

Den 17.10.2019 13.49, skrev Andy Shevchenko:
> 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@linux.intel.com>
> ---

Thanks, it's even more readable now, for me at least. And since I don't
trust my in-head C compiler/parser, I ran a test and
/sys/kernel/debug/dri/0/command returns the same for commands 04H and
09h which are the ones affected by this change.

Reviewed-by: Noralf Trønnes <noralf@tronnes.org>
Tested-by: Noralf Trønnes <noralf@tronnes.org>

This patch hasn't shown up in dri-devel patchwork, I hope it's just a
hiccup and it'll show up later since I apply patches from patchwork.
I don't see it in the mailinglist archive either, only Sean's replies,
not yours. But I do see your replies to other patches. We'll see. If not
then I'll have to export it from Windows Thunderbird and fix newlines :/

Noralf.

>  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);
>  	}
>  
>  	MIPI_DBI_DEBUG_COMMAND(*cmd, data, len);
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Noralf Trønnes]

Den 17.10.2019 18.27, skrev Noralf Trønnes:
> 
> 
> Den 17.10.2019 13.49, skrev Andy Shevchenko:
>> 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@linux.intel.com>
>> ---
> 
> Thanks, it's even more readable now, for me at least. And since I don't
> trust my in-head C compiler/parser, I ran a test and
> /sys/kernel/debug/dri/0/command returns the same for commands 04H and
> 09h which are the ones affected by this change.
> 
> Reviewed-by: Noralf Trønnes <noralf@tronnes.org>
> Tested-by: Noralf Trønnes <noralf@tronnes.org>
> 

Applied to drm-misc-next.

Noralf.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel