Re: [PATCH v1 1/9] mfd: Add core driver for Nuvoton NCT6694

[Marc Kleine-Budde <mkl@pengutronix.de>]

[-- Attachment #1: Type: text/plain, Size: 6817 bytes --]

On 28.10.2024 15:33:08, Ming Yu wrote:
> Dear Marc,
> 
> Thank you for your comments,
> 
> Marc Kleine-Budde <mkl@pengutronix.de> 於 2024年10月25日 週五 下午8:24寫道：
> >
> > On 25.10.2024 19:03:55, Ming Yu wrote:
> > > Oh! I'm sorry about that I confused the packet size.
> > > The NCT6694 bulk maximum packet size is 256 bytes,
> > > and USB High speed bulk maximum packet size is 512 bytes.
> > >
> > > Marc Kleine-Budde <mkl@pengutronix.de> 於 2024年10月25日 週五 下午6:08寫道：
> > > >
> > > > On 25.10.2024 16:08:10, Ming Yu wrote:
> > > > > > > +int nct6694_read_msg(struct nct6694 *nct6694, u8 mod, u16 offset, u16 length,
> > > > > > > +                  u8 rd_idx, u8 rd_len, unsigned char *buf)
> > > > > >
> > > > > > why not make buf a void *?
> > > > >
> > > > > [Ming] I'll change the type in the next patch.
> > > > >
> > > > > >
> > > > > > > +{
> > > > > > > +     struct usb_device *udev = nct6694->udev;
> > > > > > > +     unsigned char err_status;
> > > > > > > +     int len, packet_len, tx_len, rx_len;
> > > > > > > +     int i, ret;
> > > > > > > +
> > > > > > > +     mutex_lock(&nct6694->access_lock);
> > > > > > > +
> > > > > > > +     /* Send command packet to USB device */
> > > > > > > +     nct6694->cmd_buffer[REQUEST_MOD_IDX] = mod;
> > > > > > > +     nct6694->cmd_buffer[REQUEST_CMD_IDX] = offset & 0xFF;
> > > > > > > +     nct6694->cmd_buffer[REQUEST_SEL_IDX] = (offset >> 8) & 0xFF;
> > > > > > > +     nct6694->cmd_buffer[REQUEST_HCTRL_IDX] = HCTRL_GET;
> > > > > > > +     nct6694->cmd_buffer[REQUEST_LEN_L_IDX] = length & 0xFF;
> > > > > > > +     nct6694->cmd_buffer[REQUEST_LEN_H_IDX] = (length >> 8) & 0xFF;
> > > > > > > +
> > > > > > > +     ret = usb_bulk_msg(udev, usb_sndbulkpipe(udev, BULK_OUT_ENDPOINT),
> > > > > > > +                        nct6694->cmd_buffer, CMD_PACKET_SZ, &tx_len,
> > > > > > > +                        nct6694->timeout);
> > > > > > > +     if (ret)
> > > > > > > +             goto err;
> > > > > > > +
> > > > > > > +     /* Receive response packet from USB device */
> > > > > > > +     ret = usb_bulk_msg(udev, usb_rcvbulkpipe(udev, BULK_IN_ENDPOINT),
> > > > > > > +                        nct6694->rx_buffer, CMD_PACKET_SZ, &rx_len,
> > > > > > > +                        nct6694->timeout);
> > > > > > > +     if (ret)
> > > > > > > +             goto err;
> > > > > > > +
> > > > > > > +     err_status = nct6694->rx_buffer[RESPONSE_STS_IDX];
> > > > > > > +
> > > > > > > +     /*
> > > > > > > +      * Segmented reception of messages that exceed the size of USB bulk
> > > > > > > +      * pipe packets.
> > > > > > > +      */
> > > > > >
> > > > > > The Linux USB stack can receive bulk messages longer than the max packet size.
> > > > >
> > > > > [Ming] Since NCT6694's bulk pipe endpoint size is 128 bytes for this MFD device.
> > > > > The core will divide packet 256 bytes for high speed USB device, but
> > > > > it is exceeds
> > > > > the hardware limitation, so I am dividing it manually.
> > > >
> > > > You say the endpoint descriptor is correctly reporting it's max packet
> > > > size of 128, but the Linux USB will send packets of 256 bytes?
> > >
> > > [Ming] The endpoint descriptor is correctly reporting it's max packet
> > > size of 256, but the Linux USB may send more than 256 (max is 512)
> > > https://elixir.bootlin.com/linux/v6.11.5/source/drivers/usb/host/xhci-mem.c#L1446
> >
> > AFAIK according to the USB-2.0 spec the maximum packet size for
> > high-speed bulk transfers is fixed set to 512 bytes. Does this mean that
> > your device is a non-compliant USB device?
> 
> We will reduce the endpoint size of other interfaces to ensure that MFD device
> meets the USB2.0 spec. In other words, I will remove the code for manual
> unpacking in the next patch.

I was not talking about the driver, but your USB device. According to
the USB2.0 spec, the packet size is fixed to 512 for high-speed bulk
transfers. So your device must be able to handle 512 byte transfers or
it's a non-compliant USB device.

> > > > > > > +     for (i = 0, len = length; len > 0; i++, len -= packet_len) {
> > > > > > > +             if (len > nct6694->maxp)
> > > > > > > +                     packet_len = nct6694->maxp;
> > > > > > > +             else
> > > > > > > +                     packet_len = len;
> > > > > > > +
> > > > > > > +             ret = usb_bulk_msg(udev, usb_rcvbulkpipe(udev, BULK_IN_ENDPOINT),
> > > > > > > +                                nct6694->rx_buffer + nct6694->maxp * i,
> > > > > > > +                                packet_len, &rx_len, nct6694->timeout);
> > > > > > > +             if (ret)
> > > > > > > +                     goto err;
> > > > > > > +     }
> > > > > > > +
> > > > > > > +     for (i = 0; i < rd_len; i++)
> > > > > > > +             buf[i] = nct6694->rx_buffer[i + rd_idx];
> > > > > >
> > > > > > memcpy()?
> > > > > >
> > > > > > Or why don't you directly receive data into the provided buffer? Copying
> > > > > > of the data doesn't make it faster.
> > > > > >
> > > > > > On the other hand, receiving directly into the target buffer means the
> > > > > > target buffer must not live on the stack.
> > > > >
> > > > > [Ming] Okay! I'll change it to memcpy().
> > > >
> > > > fine!
> > > >
> > > > > This is my perspective: the data is uniformly received by the rx_bffer held
> > > > > by the MFD device. does it need to be changed?
> > > >
> > > > My question is: Why do you first receive into the nct6694->rx_buffer and
> > > > then memcpy() to the buffer provided by the caller, why don't you
> > > > directly receive into the memory provided by the caller?
> > >
> > > [Ming] Due to the bulk pipe maximum packet size limitation, I think consistently
> > > using the MFD'd dynamically allocated buffer to submit URBs will better
> > > manage USB-related operations
> >
> > The non-compliant max packet size limitation is unrelated to the
> > question which RX or TX buffer to use.
> 
> I think these two USB functions can be easily called using the buffer
> dynamically
> allocated by the MFD. However, if they transfer data directly to the
> target buffer,
> they must ensure that it is not located on the stack.

You have a high coupling between the MFD driver and the individual
drivers anyways, so why not directly use the dynamically allocated
buffer provided by the caller and get rid of the memcpy()?

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]