netdev - Re: [PATCH net-next 0/14] pull-request: can-next 2022-10-31

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]

Message-ID: <ea8f2b55-3871-8ba6-b545-d3c73a7508cc@posteo.net>
Date:   Tue,  8 Nov 2022 19:07:25 +0000
From:   Lukas Magel <lukas.magel@...teo.net>
To:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Jakub Kicinski <kuba@...nel.org>
Cc:     Marc Kleine-Budde <mkl@...gutronix.de>, netdev@...r.kernel.org,
        davem@...emloft.net, linux-can@...r.kernel.org,
        kernel@...gutronix.de
Subject: Re: [PATCH net-next 0/14] pull-request: can-next 2022-10-31

On 01.11.22 06:06, Greg Kroah-Hartman wrote:
> On Mon, Oct 31, 2022 at 08:27:14PM -0700, Jakub Kicinski wrote:
>> On Mon, 31 Oct 2022 16:43:52 +0100 Marc Kleine-Budde wrote:
>>> The first 7 patches are by Stephane Grosjean and Lukas Magel and
>>> target the peak_usb driver. Support for flashing a user defined device
>>> ID via the ethtool flash interface is added. A read only sysfs
>> nit: ethtool eeprom set != ethtool flash
>>
>>> attribute for that value is added to distinguish between devices via
>>> udev.
>> So the user can write an arbitrary u32 value into flash which then
>> persistently pops up in sysfs across reboots (as a custom attribute
>> called "user_devid")?
>>
>> I don't know.. the whole thing strikes me as odd. Greg do you have any
>> feelings about such.. solutions?
>>
>> patches 5 and 6 here:
>> https://lore.kernel.org/all/20221031154406.259857-1-mkl@pengutronix.de/
I now realize that I didn't do a sufficient job at describing the purpose of the
device ID in the patches (which I am working on improving at the moment). In
contrast to other devices (such as the ones from ETAS), at least the PCAN USB-FD
devices do not export an iSerial attribute at USB level, which makes it hard to
distinguish them if you are using multiple with the same product ID. The user
device ID is a freely configurable identifier (basically an arbitrary u8 / u32
value like you said) that can be set individually for each CAN channel of any
PEAK device and can serve as a replacement for the missing serial number. This
use case is also explicitly stated in the Windows API manual for the PEAK
devices (see page 11 in [1]). The patch series implements write support for the
value via ethtool and exports it readonly as a sysfs attribute for udev matching.
> Device-specific attributes should be in the device-specific directory,
> not burried in a class directory somewhere that is generic like this one
> is.
>
> Why isn't this an attribute of the usb device instead?

Each CAN channel of a PEAK device can have its own device ID, meaning that there
is a potential one-to-n mapping between USB device and device IDs. I can see how
the name might appear confusing in that regard, we chose it to be consistent
with the API description put out by PEAK (also see [1]).

> And there's no need to reorder the .h file includes in patch 06 while
> you are adding a sysfs entry, that should be a separate commit, right?
I have split this into a separate commit.
> Also, the line:
>
> +	.attrs	= (struct attribute **)peak_usb_sysfs_attrs,
>
> Is odd, there should never be a need to cast anything like this if you
> are doing things properly.
I have removed the const modifier from the struct as well as the cast.
> So this still needs work, sorry.
>
> thanks,
>
> greg k-h
>
Please let me know if you require further changes to the patch series or want
the attribute to be renamed.

Regards,


Lukas

[1] See PCAN-Parameter_Documentation.pdf in
https://www.peak-system.com/fileadmin/media/files/pcan-basic.zip