linux-kernel - Re: [PATCH v2] hwmon: add POWER-Z driver

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]

Date:   Thu, 31 Aug 2023 20:03:37 +0200
From:   Thomas Weißschuh <linux@...ssschuh.net>
To:     Guenter Roeck <linux@...ck-us.net>
Cc:     Jean Delvare <jdelvare@...e.com>, linux-kernel@...r.kernel.org,
        linux-hwmon@...r.kernel.org
Subject: Re: [PATCH v2] hwmon: add POWER-Z driver

Hi Guenter,

thanks for your review!

Ack to most of your points.

> [..]

> > +
> > +#define DRIVER_NAME	"powerz"
> > +#define POWERZ_EP_CMD_OUT	0x01
> > +#define POWERZ_EP_DATA_IN	0x81
> > +
> > +struct powerz_sensor_data {
> > +	u8 _unknown_1[8];
> > +	__le32 Vbus;
> 
> CHECK: Avoid CamelCase: <Vbus>
> #160: FILE: drivers/hwmon/powerz.c:18:
> +	__le32 Vbus;
> 
> Please run your patches through checkpatch --strict.

I did. Weird that it didn't show. I'll investigate.
(And fix it)

> 
> > +	__le32 Ibus;
> > +	__le32 Vbus_avg;
> > +	__le32 Ibus_avg;
> > +	u8 _unknown_2[8];
> > +	u8 temp[2];
> > +	__le16 cc1;
> > +	__le16 cc2;
> > +	__le16 dp;
> > +	__le16 dm;
> > +	u8 _unknown_3[6];
> > +} __packed;
> > +

> [..]

> > +static int powerz_read(struct device *dev, enum hwmon_sensor_types type, u32 attr,
> > +		       int channel, long *val)
> > +{
> > +	struct usb_interface *intf = to_usb_interface(dev->parent);
> > +	struct usb_device *udev = interface_to_usbdev(intf);
> > +	struct powerz_sensor_data *data;
> > +	struct powerz_usb_ctx *ctx;
> > +
> > +	ctx = kmalloc(sizeof(*ctx), GFP_KERNEL);
> > +	if (!ctx)
> > +		return -ENOMEM;
> > +
> 
> I think it would be much better to allocate ctx once as part of
> struct powerz_priv and keep it around. Sure, that means that this
> function requires a lock, but I don't see that as problem (and who
> knows how the device reacts to multiple pending usb transactions).
> 
> You'd need to allocate transfer_buffer separately because it needs to be
> dma aligned, but that should not be a problem either.

What is your opinion on making the transfer buffer the first member of
struct powerz_priv? It would simplify the code and still provide a
DMA-capable buffer.

> [..]

> > +static int powerz_probe(struct usb_interface *intf, const struct usb_device_id *id)
> > +{
> > +	struct usb_device *udev = interface_to_usbdev(intf);
> > +	struct powerz_priv *priv;
> > +	struct device *parent;
> > +	const char *name;
> > +	int ret;
> > +
> > +	parent = &intf->dev;
> > +
> > +	priv = devm_kzalloc(parent, sizeof(*priv), GFP_KERNEL);
> > +	if (!priv)
> > +		return -ENOMEM;
> > +
> > +	name = devm_hwmon_sanitize_name(parent, udev->product ?: DRIVER_NAME);
> 
> Why not just use DRIVER_NAME ? This would be much more consistent.

I liked the more detailed name better.
But if you prefer otherwise I'll simplify it.