linux-kernel - Re: [PATCH 2/2] usb: misc: usb5744: Add support for USB hub controller

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Wed, 10 Feb 2021 12:03:56 +0100
From:   Michal Simek <michal.simek@...inx.com>
To:     Andrew Lunn <andrew@...n.ch>,
        Michal Simek <michal.simek@...inx.com>
CC:     <linux-kernel@...r.kernel.org>, <monstr@...str.eu>,
        <git@...inx.com>, <linux-usb@...r.kernel.org>,
        "Alexander A. Klimov" <grandmaster@...klimov.de>,
        Piyush Mehta <piyush.mehta@...inx.com>,
        "Masahiro Yamada" <masahiroy@...nel.org>,
        Al Cooper <alcooperx@...il.com>,
        Alan Stern <stern@...land.harvard.edu>,
        Bastien Nocera <hadess@...ess.net>,
        "Greg Kroah-Hartman" <gregkh@...uxfoundation.org>,
        <linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH 2/2] usb: misc: usb5744: Add support for USB hub
 controller

Hi Andrew,

On 2/10/21 3:52 AM, Andrew Lunn wrote:
> On Tue, Feb 09, 2021 at 10:53:20AM +0100, Michal Simek wrote:
>> +static int usb5744_i2c_probe(struct i2c_client *client,
>> +			     const struct i2c_device_id *id)
>> +{
>> +	struct device *dev = &client->dev;
>> +	int ret;
>> +
>> +	/* Trigger gpio reset to the hub. */
>> +	ret = usb5744_init_hw(dev);
>> +	if (ret)
>> +		return ret;
>> +
>> +	/* Send SMBus command to boot hub. */
>> +	ret = i2c_smbus_write_word_data(client, 0xAA, swab16(0x5600));
> 
> Hi Michal
> 
> This is not my area of the kernel. But that swab16() stood out, and
> made me wonder about endianness. Will this work correctly on big and
> little endian hosts?

thanks for bringing this up. I didn't test it on BE system.

I have grepped the kernel

[linux](xnext/usb5744)$ git grep i2c_smbus_write_word_data | grep swab
drivers/media/i2c/uda1342.c:17:	i2c_smbus_write_word_data(client, reg,
swab16(value));
drivers/media/i2c/vpx3220.c:97:	if (i2c_smbus_write_word_data(client,
0x27, swab16(fpaddr)) == -1) {
drivers/media/i2c/vpx3220.c:106:	if (i2c_smbus_write_word_data(client,
0x28, swab16(data)) == -1) {
drivers/media/i2c/vpx3220.c:120:	if (i2c_smbus_write_word_data(client,
0x26, swab16(fpaddr)) == -1) {
drivers/usb/misc/usb5744.c:50:	ret = i2c_smbus_write_word_data(client,
0xAA, swab16(0x5600));
include/linux/i2c.h:168:	return i2c_smbus_write_word_data(client,
command, swab16(value));

And last one is interesting

 164 static inline s32
 165 i2c_smbus_write_word_swapped(const struct i2c_client *client,
 166                              u8 command, u16 value)
 167 {
 168         return i2c_smbus_write_word_data(client, command,
swab16(value));
 169 }

And this function is also used

[linux](xnext/usb5744)$ git grep i2c_smbus_write_word_swapped | wc -l
76

I think it would be the best to test it and see if this code works on BE
but I need to prepare it first.
And current code is aligned with others but it doesn't mean that it is
correct.

Thanks,
Michal