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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20221104130857.amzwa2mzmwhbljmk@pengutronix.de>
Date:   Fri, 4 Nov 2022 14:08:57 +0100
From:   Marc Kleine-Budde <mkl@...gutronix.de>
To:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc:     Jakub Kicinski <kuba@...nel.org>, 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.2022 06:06:13, 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/
> 
> 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?

What about:

| /sys/devices/pci0000:00/0000:00:13.0/usb1/1-1/1-1:1.0/device_id

> 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?

ACK

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

After marking the struct attribute not as const, we can remove the cast:

| --- a/drivers/net/can/usb/peak_usb/pcan_usb_core.c
| +++ b/drivers/net/can/usb/peak_usb/pcan_usb_core.c
| @@ -64,14 +64,14 @@ static ssize_t user_devid_show(struct device *dev, struct device_attribute *attr
|  }
|  static DEVICE_ATTR_RO(user_devid);
|  
| -static const struct attribute *peak_usb_sysfs_attrs[] = {
| +static struct attribute *peak_usb_sysfs_attrs[] = {
|         &dev_attr_user_devid.attr,
|         NULL,
|  };
|  
|  static const struct attribute_group peak_usb_sysfs_group = {
|         .name   = "peak_usb",
| -       .attrs  = (struct attribute **)peak_usb_sysfs_attrs,
| +       .attrs  = peak_usb_sysfs_attrs,
|  };
|  
|  /*

But this code is obsolete, if we move the sysfs entry into the USB
device.

> So this still needs work, sorry.

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)