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

Message-ID: <6883223f-20ec-e5be-8015-256198f1305f@roeck-us.net>
Date:   Thu, 31 Aug 2023 13:02:32 -0700
From:   Guenter Roeck <linux@...ck-us.net>
To:     Thomas Weißschuh <linux@...ssschuh.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

On 8/31/23 11:03, Thomas Weißschuh wrote:
> 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.
> 

Sure, works for me.

>> [..]
> 
>>> +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.
> 

I think it just confuses users because it isn't well defined.

Thanks,
Guenter