[BUG] device_property_read_u16 reads out only zero

[BUG] device_property_read_u16 reads out only zero

[Peter Geis]

Good Evening,

I seem to have come across a strange bug with drivers/base/property.c
while expanding the new cyttsp5 driver to handle device-tree
overrides.

With the property:
touchscreen-size-x = <1863>;
The following code:
u32 test_u32 = 32; /* canary to catch writing a zero */
u16 test_u16 = 16; /* canary to catch writing a zero */
int ret;

ret = device_property_read_u32(ts->dev, "touchscreen-size-x", &test_u32);
if(ret)
dev_err(ts->dev, "read_u32 failed ret: %d\n", ret);

ret = device_property_read_u16(ts->dev, "touchscreen-size-x", &test_u16);
if(ret)
dev_err(ts->dev, "read_u16 failed ret: %d\n", ret);

dev_err(ts->dev, "read_u32: %d, read_u16: %d\n", test_u32, test_u16);

returns the following:
[    1.010876] cyttsp5 5-0024: read_u32: 1863, read_u16: 0

This was as of 5.16-rc8, using the
gcc-arm-10.2-2020.11-x86_64-aarch64-none-linux-gnu compiler.
I honestly am at a loss here, any insight you can provide here would
be appreciated.

Very Respectfully,
Peter Geis

Re: [BUG] device_property_read_u16 reads out only zero

[Rob Herring]

On Fri, Jan 14, 2022 at 05:48:52PM -0500, Peter Geis wrote:
> Good Evening,
> 
> I seem to have come across a strange bug with drivers/base/property.c
> while expanding the new cyttsp5 driver to handle device-tree
> overrides.
> 
> With the property:
> touchscreen-size-x = <1863>;
> The following code:
> u32 test_u32 = 32; /* canary to catch writing a zero */
> u16 test_u16 = 16; /* canary to catch writing a zero */
> int ret;
> 
> ret = device_property_read_u32(ts->dev, "touchscreen-size-x", &test_u32);
> if(ret)
> dev_err(ts->dev, "read_u32 failed ret: %d\n", ret);
> 
> ret = device_property_read_u16(ts->dev, "touchscreen-size-x", &test_u16);
> if(ret)
> dev_err(ts->dev, "read_u16 failed ret: %d\n", ret);
> 
> dev_err(ts->dev, "read_u32: %d, read_u16: %d\n", test_u32, test_u16);
> 
> returns the following:
> [    1.010876] cyttsp5 5-0024: read_u32: 1863, read_u16: 0
> 
> This was as of 5.16-rc8, using the
> gcc-arm-10.2-2020.11-x86_64-aarch64-none-linux-gnu compiler.
> I honestly am at a loss here, any insight you can provide here would
> be appreciated.

The property "touchscreen-size-x" is a u32. Calling 
device_property_read_u16 is an error though one is not returned here. 
You get 0 because that is what the first 2 bytes of the property 
contain. DT data is big-endian.

I suspect making this a hard error would break some users, but we could 
try a WARN.

Rob

Re: [BUG] device_property_read_u16 reads out only zero

[Peter Geis]

On Fri, Jan 21, 2022 at 5:19 PM Rob Herring <robh@kernel.org> wrote:
>
> On Fri, Jan 14, 2022 at 05:48:52PM -0500, Peter Geis wrote:
> > Good Evening,
> >
> > I seem to have come across a strange bug with drivers/base/property.c
> > while expanding the new cyttsp5 driver to handle device-tree
> > overrides.
> >
> > With the property:
> > touchscreen-size-x = <1863>;
> > The following code:
> > u32 test_u32 = 32; /* canary to catch writing a zero */
> > u16 test_u16 = 16; /* canary to catch writing a zero */
> > int ret;
> >
> > ret = device_property_read_u32(ts->dev, "touchscreen-size-x", &test_u32);
> > if(ret)
> > dev_err(ts->dev, "read_u32 failed ret: %d\n", ret);
> >
> > ret = device_property_read_u16(ts->dev, "touchscreen-size-x", &test_u16);
> > if(ret)
> > dev_err(ts->dev, "read_u16 failed ret: %d\n", ret);
> >
> > dev_err(ts->dev, "read_u32: %d, read_u16: %d\n", test_u32, test_u16);
> >
> > returns the following:
> > [    1.010876] cyttsp5 5-0024: read_u32: 1863, read_u16: 0
> >
> > This was as of 5.16-rc8, using the
> > gcc-arm-10.2-2020.11-x86_64-aarch64-none-linux-gnu compiler.
> > I honestly am at a loss here, any insight you can provide here would
> > be appreciated.
>
> The property "touchscreen-size-x" is a u32. Calling
> device_property_read_u16 is an error though one is not returned here.
> You get 0 because that is what the first 2 bytes of the property
> contain. DT data is big-endian.

I figured this was the case, but I was hopeful the operators would be
smart enough to handle endian translations.
Wouldn't all DT numeric properties be u32, meaning
device_property_read_u16 and device_property_read_u8 are meaningless
on little endian devices?
Or is there a way to force smaller values in the DT?

>
> I suspect making this a hard error would break some users, but we could
> try a WARN.

I don't suspect it would be trivial to implement endian translation
for these functions?

>
> Rob

Always,
Peter

Re: [BUG] device_property_read_u16 reads out only zero

[Rob Herring]

On Fri, Jan 21, 2022 at 7:27 PM Peter Geis <pgwipeout@gmail.com> wrote:
>
> On Fri, Jan 21, 2022 at 5:19 PM Rob Herring <robh@kernel.org> wrote:
> >
> > On Fri, Jan 14, 2022 at 05:48:52PM -0500, Peter Geis wrote:
> > > Good Evening,
> > >
> > > I seem to have come across a strange bug with drivers/base/property.c
> > > while expanding the new cyttsp5 driver to handle device-tree
> > > overrides.
> > >
> > > With the property:
> > > touchscreen-size-x = <1863>;
> > > The following code:
> > > u32 test_u32 = 32; /* canary to catch writing a zero */
> > > u16 test_u16 = 16; /* canary to catch writing a zero */
> > > int ret;
> > >
> > > ret = device_property_read_u32(ts->dev, "touchscreen-size-x", &test_u32);
> > > if(ret)
> > > dev_err(ts->dev, "read_u32 failed ret: %d\n", ret);
> > >
> > > ret = device_property_read_u16(ts->dev, "touchscreen-size-x", &test_u16);
> > > if(ret)
> > > dev_err(ts->dev, "read_u16 failed ret: %d\n", ret);
> > >
> > > dev_err(ts->dev, "read_u32: %d, read_u16: %d\n", test_u32, test_u16);
> > >
> > > returns the following:
> > > [    1.010876] cyttsp5 5-0024: read_u32: 1863, read_u16: 0
> > >
> > > This was as of 5.16-rc8, using the
> > > gcc-arm-10.2-2020.11-x86_64-aarch64-none-linux-gnu compiler.
> > > I honestly am at a loss here, any insight you can provide here would
> > > be appreciated.
> >
> > The property "touchscreen-size-x" is a u32. Calling
> > device_property_read_u16 is an error though one is not returned here.
> > You get 0 because that is what the first 2 bytes of the property
> > contain. DT data is big-endian.
>
> I figured this was the case, but I was hopeful the operators would be
> smart enough to handle endian translations.
> Wouldn't all DT numeric properties be u32, meaning
> device_property_read_u16 and device_property_read_u8 are meaningless
> on little endian devices?

No.

> Or is there a way to force smaller values in the DT?

Yes. [ 0 1 2 ] notation is 8-bit. Or you can use the /bits/ 8|16|64 directive.

> > I suspect making this a hard error would break some users, but we could
> > try a WARN.
>
> I don't suspect it would be trivial to implement endian translation
> for these functions?

They do endian translation already. In your case byte 0 and byte 1 are swapped.

The DT data is purely a byte array once it's in the dtb. It's up to
the caller with specific knowledge about a property to know how to
interpret it. It would be nice if all the size and type information
was maintained in the dtb format, but it's not and no one has stepped
up to do a new format (changing would be painful too).

Rob