Re: [PATCH v7 06/10] mfd: nct6694: Transfer command payloads via a dedicated DMA buffer

[Lee Jones]

On Thu, 03 Sep 2026, Lee Jones wrote:

> On Fri, 21 Aug 2026, a0282524688@xxxxxxxxx wrote:
> 
> > From: Ming Yu <a0282524688@xxxxxxxxx>
> > 
> > The transport hands the caller's payload buffer straight to
> > usb_bulk_msg(). Sub-device drivers embed those buffers in their private
> > data structures, so they are neither cacheline aligned nor exclusively
> > owned by the transfer. On non-coherent architectures, mapping such a
> > buffer for DMA can corrupt the unrelated fields sharing its cachelines.
> > 
> > Transfer the payload through a buffer owned by the transport instead,
> > and reject commands exceeding the maximum firmware packet size.
> > 
> > Fixes: 51dad33ede63 ("mfd: Add core driver for Nuvoton NCT6694")
> > Signed-off-by: Ming Yu <a0282524688@xxxxxxxxx>
> > ---
> > Changes in v7:
> > - New patch. Fixes the DMA-unsafe use of caller-owned payload buffers
> >   reported on v6 patch 6/7.
> > 
> >  drivers/mfd/nct6694-usb.c   | 38 +++++++++++++++++++++++++++----------
> >  include/linux/mfd/nct6694.h |  3 +++
> >  2 files changed, 31 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/mfd/nct6694-usb.c b/drivers/mfd/nct6694-usb.c
> > index 2289ebfde7fa..793ce54c02aa 100644
> > --- 'a/drivers/mfd/nct6694-usb.c'
> > +++ 'b/drivers/mfd/nct6694-usb.c'
> > @@ -38,6 +38,7 @@ struct nct6694_usb_data {
> >  	struct urb *int_in_urb;
> >  	struct usb_device *udev;
> >  	union nct6694_usb_msg *usb_msg;
> > +	void *xfer_buf;
> >  	__le32 *int_buffer;
> 
> There are a bunch of buffers used in this file and I'm losing track of
> which one is which.  Please consolidate on the nomenclature for them
> all.
> 
> Ins, outs, payloads, xfers, etc.  Make a decision and stick to it throughout.

Same with 'len' - label them all please.

> >  };
> >  
> > @@ -120,8 +121,12 @@ int nct6694_usb_read_msg(struct nct6694 *nct6694,
> >  	struct nct6694_usb_data *udata = nct6694->priv;
> >  	union nct6694_usb_msg *msg = udata->usb_msg;
> >  	struct usb_device *udev = udata->udev;
> > +	u16 len = le16_to_cpu(cmd_hd->len);
> >  	int tx_len, rx_len, ret;
> >  
> > +	if (len > NCT6694_MAX_PACKET_SIZE)
> > +		return -EINVAL;
> > +
> >  	guard(mutex)(&udata->access_lock);
> >  
> >  	memcpy(&msg->cmd_header, cmd_hd, sizeof(*cmd_hd));
> > @@ -140,17 +145,19 @@ int nct6694_usb_read_msg(struct nct6694 *nct6694,
> >  		return ret;
> >  
> >  	/* Receive data packet from USB device */
> > -	ret = usb_bulk_msg(udev, usb_rcvbulkpipe(udev, NCT6694_BULK_IN_EP), buf,
> > -			   le16_to_cpu(cmd_hd->len), &rx_len, NCT6694_URB_TIMEOUT);
> > +	ret = usb_bulk_msg(udev, usb_rcvbulkpipe(udev, NCT6694_BULK_IN_EP), udata->xfer_buf,
> > +			   len, &rx_len, NCT6694_URB_TIMEOUT);
> >  	if (ret)
> >  		return ret;
> >  
> > -	if (rx_len != le16_to_cpu(cmd_hd->len)) {
> > +	if (rx_len != len) {
> >  		dev_err(nct6694->dev, "Expected received length %d, but got %d\n",
> > -			le16_to_cpu(cmd_hd->len), rx_len);
> > +			len, rx_len);
> >  		return -EIO;
> >  	}
> >  
> > +	memcpy(buf, udata->xfer_buf, len);
> > +
> >  	return nct6694_usb_err_handling(nct6694, msg->response_header.sts);
> >  }
> >  EXPORT_SYMBOL_GPL(nct6694_usb_read_msg);
> > @@ -173,12 +180,17 @@ int nct6694_usb_write_msg(struct nct6694 *nct6694,
> >  	struct nct6694_usb_data *udata = nct6694->priv;
> >  	union nct6694_usb_msg *msg = udata->usb_msg;
> >  	struct usb_device *udev = udata->udev;
> > +	u16 len = le16_to_cpu(cmd_hd->len);
> >  	int tx_len, rx_len, ret;
> >  
> > +	if (len > NCT6694_MAX_PACKET_SIZE)
> > +		return -EINVAL;
> > +
> >  	guard(mutex)(&udata->access_lock);
> >  
> >  	memcpy(&msg->cmd_header, cmd_hd, sizeof(*cmd_hd));
> >  	msg->cmd_header.hctrl = NCT6694_HCTRL_SET;
> > +	memcpy(udata->xfer_buf, buf, len);
> >  
> >  	/* Send command packet to USB device */
> >  	ret = usb_bulk_msg(udev, usb_sndbulkpipe(udev, NCT6694_BULK_OUT_EP), &msg->cmd_header,
> > @@ -187,8 +199,8 @@ int nct6694_usb_write_msg(struct nct6694 *nct6694,
> >  		return ret;
> >  
> >  	/* Send data packet to USB device */
> > -	ret = usb_bulk_msg(udev, usb_sndbulkpipe(udev, NCT6694_BULK_OUT_EP), buf,
> > -			   le16_to_cpu(cmd_hd->len), &tx_len, NCT6694_URB_TIMEOUT);
> > +	ret = usb_bulk_msg(udev, usb_sndbulkpipe(udev, NCT6694_BULK_OUT_EP), udata->xfer_buf,
> > +			   len, &tx_len, NCT6694_URB_TIMEOUT);
> >  	if (ret)
> >  		return ret;
> >  
> > @@ -199,17 +211,19 @@ int nct6694_usb_write_msg(struct nct6694 *nct6694,
> >  		return ret;
> >  
> >  	/* Receive data packet from USB device */
> > -	ret = usb_bulk_msg(udev, usb_rcvbulkpipe(udev, NCT6694_BULK_IN_EP), buf,
> > -			   le16_to_cpu(cmd_hd->len), &rx_len, NCT6694_URB_TIMEOUT);
> > +	ret = usb_bulk_msg(udev, usb_rcvbulkpipe(udev, NCT6694_BULK_IN_EP), udata->xfer_buf,
> > +			   len, &rx_len, NCT6694_URB_TIMEOUT);
> >  	if (ret)
> >  		return ret;
> >  
> > -	if (rx_len != le16_to_cpu(cmd_hd->len)) {
> > +	if (rx_len != len) {
> >  		dev_err(nct6694->dev, "Expected transmitted length %d, but got %d\n",
> > -			le16_to_cpu(cmd_hd->len), rx_len);
> > +			len, rx_len);
> >  		return -EIO;
> >  	}
> >  
> > +	memcpy(buf, udata->xfer_buf, len);
> > +
> >  	return nct6694_usb_err_handling(nct6694, msg->response_header.sts);
> >  }
> >  EXPORT_SYMBOL_GPL(nct6694_usb_write_msg);
> > @@ -270,6 +284,10 @@ static int nct6694_usb_probe(struct usb_interface *iface,
> >  	if (!udata->usb_msg)
> >  		return -ENOMEM;
> >  
> > +	udata->xfer_buf = devm_kzalloc(dev, NCT6694_MAX_PACKET_SIZE, GFP_KERNEL);
> > +	if (!udata->xfer_buf)
> > +		return -ENOMEM;
> > +
> >  	udata->int_buffer = devm_kzalloc(dev, sizeof(*udata->int_buffer), GFP_KERNEL);
> >  	if (!udata->int_buffer)
> >  		return -ENOMEM;
> > diff --git a/include/linux/mfd/nct6694.h b/include/linux/mfd/nct6694.h
> > index 853b1530755d..cb311e58a437 100644
> > --- a/include/linux/mfd/nct6694.h
> > +++ b/include/linux/mfd/nct6694.h
> > @@ -29,6 +29,9 @@ struct mfd_cell;
> >  #define NCT6694_HCTRL_SET	0x40
> >  #define NCT6694_HCTRL_GET	0x80
> >  
> > +/* Maximum payload length the firmware accepts in a single command */
> > +#define NCT6694_MAX_PACKET_SIZE	0x3F0
> > +
> >  enum nct6694_irq_id {
> >  	NCT6694_IRQ_GPIO0 = 0,
> >  	NCT6694_IRQ_GPIO1,
> > -- 
> > 2.34.1
> > 
> 
> -- 
> Lee Jones

-- 
Lee Jones