Re: [PATCH v1] mfd: viperboard: fix native fields type in structures as little-endiand

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:

New issues:
- [Medium] Incomplete fix: vprbrd_i2c_write_hdr was not updated as claimed by the commit message.
- [Low] Commit message violates MFD subsystem capitalization guidelines.

Pre-existing issues:
- [High] vprbrd_probe() hard-codes PLATFORM_DEVID_NONE (-1), preventing multiple devices from probing.
- [High] vprbrd_i2c_xfer() incorrectly returns 0 on success instead of the number of transferred messages.
- [High] Ignored return value of i2c_add_adapter() can lead to kernel crash on module removal.
- [High] Buffer overwrite during large I2C reads causes corrupted USB command headers to be sent.
- [High] DMA buffer shares a cacheline with a mutex, causing data corruption on non-coherent architectures.
--

--- Patch [1]: [PATCH v1] mfd: viperboard: fix native fields type in structures as little-endiand ---
commit cb2959fa53e5e3eb2ffeb7ffb3dcd0156c7fb289
Author: Mikhail Lukianchikov <[email protected]>

> mfd: viperboard: fix native fields type in structures as little-endiand

[Severity: Low]
This isn't a bug, but does the commit message violate the MFD subsystem
capitalization guidelines? The word 'fix' is lowercase, but the guidelines
request capitalizing the description after the subsystem prefix.

This commit updates 16-bit fields in raw USB packet structures to use
__le16 instead of u16 to fix sparse warnings about endianness mismatches.

> diff --git a/include/linux/mfd/viperboard.h b/include/linux/mfd/viperboard.h
> index 193452848c047..3e700cce5cb4d 100644
> --- a/include/linux/mfd/viperboard.h
> +++ b/include/linux/mfd/viperboard.h
> @@ -62,15 +62,15 @@ struct vprbrd_i2c_write_hdr {

[Severity: Medium]
Was struct vprbrd_i2c_write_hdr intended to be updated in this patch?

The commit message states that the fields in this structure require
Little-Endian byte-order, but the structure remains unpatched as u16,
leaving the sparse warning unresolved for the write path.

>  
>  struct vprbrd_i2c_read_hdr {
>  	u8 cmd;
> -	u16 addr;
> +	__le16 addr;

[ ... ]

[Severity: High]
This isn't a bug introduced by this patch, but does vprbrd_probe() prevent
multiple devices from probing by forcing static naming?

drivers/mfd/viperboard.c:vprbrd_probe() {
    ...
	ret = mfd_add_devices(&interface->dev, -1, vprbrd_devs,
				ARRAY_SIZE(vprbrd_devs), NULL, 0, NULL);
    ...
}

Passing PLATFORM_DEVID_NONE (-1) instead of PLATFORM_DEVID_AUTO assigns static
names to the subdevices. If a second device is connected, won't this cause a
sysfs collision and probe failure?

[Severity: High]
This isn't a bug introduced by this patch, but does vprbrd_i2c_xfer() return the
correct value on success?

drivers/i2c/busses/i2c-viperboard.c:vprbrd_i2c_xfer() {
    ...
		mutex_unlock(&vb->lock);
	}
	return 0;
error:
	mutex_unlock(&vb->lock);
	return error;
}

The I2C subsystem requires the master_xfer callback to return the number of
successfully processed messages on success. Will returning 0 cause the I2C core
and client drivers to erroneously believe the transfer failed?

[Severity: High]
This isn't a bug introduced by this patch, but can the ignored return value of
i2c_add_adapter() lead to a kernel crash on module removal?

drivers/i2c/busses/i2c-viperboard.c:vprbrd_i2c_probe() {
    ...
	/* attach to i2c layer */
	i2c_add_adapter(&vb_i2c->i2c);

	platform_set_drvdata(pdev, vb_i2c);

	return 0;
}

If registration fails, the driver probe still succeeds. Upon device removal,
vprbrd_i2c_remove() will unconditionally call i2c_del_adapter(). Does this
attempt to unregister a device that was never fully added, leading to list
corruption or a crash?

[Severity: High]
This isn't a bug introduced by this patch, but is there a buffer overwrite
during large I2C reads that causes corrupted USB command headers to be sent?

drivers/i2c/busses/i2c-viperboard.c:vprbrd_i2c_read() {
    ...
		/* second read transfer if neccessary */
		if (len2 > 0) {
			ret = vprbrd_i2c_receive(vb->usb_dev, rmsg, len2);
			if (ret < 0)
				return ret;
			/* copy the received data */
			memcpy(msg->buf + start + 512, rmsg, len2);
		}
    ...
}

The rmsg buffer is used to send the read command header and receive the payload.
The first USB IN transfer overwrites the command header with incoming I2C data.
For reads requiring a second chunk, is the overwritten garbage data sent as a
command header to the device?

[Severity: High]
This isn't a bug introduced by this patch, but does the DMA buffer share a
cacheline with a mutex, risking memory corruption on non-coherent architectures?

include/linux/mfd/viperboard.h:struct vprbrd {
    ...
	struct usb_device *usb_dev; /* the usb device for this device */
	struct mutex lock;
	u8 buf[sizeof(struct vprbrd_i2c_write_msg)];
	struct platform_device pdev;
};

The buf array is mapped for USB DMA and placed immediately after the lock mutex.
If the mutex is accessed while DMA is active, will the cacheline be dirtied by
the CPU and overwrite the DMA data, or conversely, will DMA cache invalidation
destroy the CPU's modifications to the mutex waitqueue?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1