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

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]
Message-ID: <CAOoeyxW5QwPMGAYCWhQDtZwJJLG5xj9HXpL3-cduRSgF+4VHhg@mail.gmail.com>
Date: Mon, 28 Oct 2024 15:33:08 +0800
From: Ming Yu <a0282524688@...il.com>
To: Marc Kleine-Budde <mkl@...gutronix.de>
Cc: tmyu0@...oton.com, lee@...nel.org, linus.walleij@...aro.org, brgl@...ev.pl, 
	andi.shyti@...nel.org, mailhol.vincent@...adoo.fr, andrew+netdev@...n.ch, 
	davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org, pabeni@...hat.com, 
	wim@...ux-watchdog.org, linux@...ck-us.net, jdelvare@...e.com, 
	jic23@...nel.org, lars@...afoo.de, ukleinek@...nel.org, 
	alexandre.belloni@...tlin.com, linux-kernel@...r.kernel.org, 
	linux-gpio@...r.kernel.org, linux-i2c@...r.kernel.org, 
	linux-can@...r.kernel.org, netdev@...r.kernel.org, 
	linux-watchdog@...r.kernel.org, linux-hwmon@...r.kernel.org, 
	linux-iio@...r.kernel.org, linux-pwm@...r.kernel.org, 
	linux-rtc@...r.kernel.org
Subject: Re: [PATCH v1 1/9] mfd: Add core driver for Nuvoton NCT6694

Dear Marc,

Thank you for your comments,

Marc Kleine-Budde <mkl@...gutronix.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@...gutronix.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.

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

>
> 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   |

Ming