On 09.03.2022 23:05:57, Vincent Mailhol wrote:
> On Wed. 9 Mar 2022 à 22:39, Marc Kleine-Budde <mkl@xxxxxxxxxxxxxx> wrote:
> > From: Peter Fink <pfink@xxxxxxxxxxxx>
> >
> > Modify struct gs_host_frame to make use of a union and
> > DECLARE_FLEX_ARRAY to be able to store different data (lengths), which
> > will be added in later commits.
> >
> > Store the gs_host_frame length in TX direction (host -> device) in
> > struct gs_can::hf_size_tx and RX direction (device -> host) in struct
> > gs_usb::hf_size_rx so it must be calculated only once.
> >
> > Signed-off-by: Peter Fink <pfink@xxxxxxxxxxxx>
> > Signed-off-by: Marc Kleine-Budde <mkl@xxxxxxxxxxxxxx>
> > ---
> > drivers/net/can/usb/gs_usb.c | 37 +++++++++++++++++++++++++-----------
> > 1 file changed, 26 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/net/can/usb/gs_usb.c b/drivers/net/can/usb/gs_usb.c
> > index 4bc10264005b..1fe9d9f08c17 100644
> > --- a/drivers/net/can/usb/gs_usb.c
> > +++ b/drivers/net/can/usb/gs_usb.c
> > @@ -146,6 +146,10 @@ struct gs_device_bt_const {
> >
> > #define GS_CAN_FLAG_OVERFLOW BIT(0)
> >
> > +struct classic_can {
> > + u8 data[8];
> > +} __packed;
> > +
> > struct gs_host_frame {
> > u32 echo_id;
> > __le32 can_id;
> > @@ -155,7 +159,9 @@ struct gs_host_frame {
> > u8 flags;
> > u8 reserved;
> >
> > - u8 data[8];
> > + union {
> > + DECLARE_FLEX_ARRAY(struct classic_can, classic_can);
> > + };
> > } __packed;
> > /* The GS USB devices make use of the same flags and masks as in
> > * linux/can.h and linux/can/error.h, and no additional mapping is necessary.
> > @@ -187,6 +193,8 @@ struct gs_can {
> > struct can_bittiming_const bt_const;
> > unsigned int channel; /* channel number */
> >
> > + unsigned int hf_size_tx;
> > +
> > /* This lock prevents a race condition between xmit and receive. */
> > spinlock_t tx_ctx_lock;
> > struct gs_tx_context tx_context[GS_MAX_TX_URBS];
> > @@ -200,6 +208,7 @@ struct gs_usb {
> > struct gs_can *canch[GS_MAX_INTF];
> > struct usb_anchor rx_submitted;
> > struct usb_device *udev;
> > + unsigned int hf_size_rx;
> > u8 active_channels;
> > };
> >
> > @@ -343,7 +352,7 @@ static void gs_usb_receive_bulk_callback(struct urb *urb)
> > cf->can_id = le32_to_cpu(hf->can_id);
> >
> > can_frame_set_cc_len(cf, hf->can_dlc, dev->can.ctrlmode);
> > - memcpy(cf->data, hf->data, 8);
> > + memcpy(cf->data, hf->classic_can->data, 8);
> >
> > /* ERROR frames tell us information about the controller */
> > if (le32_to_cpu(hf->can_id) & CAN_ERR_FLAG)
> > @@ -398,7 +407,7 @@ static void gs_usb_receive_bulk_callback(struct urb *urb)
> > resubmit_urb:
> > usb_fill_bulk_urb(urb, usbcan->udev,
> > usb_rcvbulkpipe(usbcan->udev, GSUSB_ENDPOINT_IN),
> > - hf, sizeof(struct gs_host_frame),
> > + hf, dev->parent->hf_size_rx,
> > gs_usb_receive_bulk_callback, usbcan);
> >
> > rc = usb_submit_urb(urb, GFP_ATOMIC);
> > @@ -485,7 +494,7 @@ static netdev_tx_t gs_can_start_xmit(struct sk_buff *skb,
> > if (!urb)
> > goto nomem_urb;
> >
> > - hf = usb_alloc_coherent(dev->udev, sizeof(*hf), GFP_ATOMIC,
> > + hf = usb_alloc_coherent(dev->udev, dev->hf_size_tx, GFP_ATOMIC,
> > &urb->transfer_dma);
> > if (!hf) {
> > netdev_err(netdev, "No memory left for USB buffer\n");
> > @@ -509,11 +518,11 @@ static netdev_tx_t gs_can_start_xmit(struct sk_buff *skb,
> > hf->can_id = cpu_to_le32(cf->can_id);
> > hf->can_dlc = can_get_cc_dlc(cf, dev->can.ctrlmode);
> >
> > - memcpy(hf->data, cf->data, cf->len);
> > + memcpy(hf->classic_can->data, cf->data, cf->len);
> >
> > usb_fill_bulk_urb(urb, dev->udev,
> > usb_sndbulkpipe(dev->udev, GSUSB_ENDPOINT_OUT),
> > - hf, sizeof(*hf),
> > + hf, dev->hf_size_tx,
> > gs_usb_xmit_callback, txc);
> >
> > urb->transfer_flags |= URB_NO_TRANSFER_DMA_MAP;
> > @@ -531,8 +540,8 @@ static netdev_tx_t gs_can_start_xmit(struct sk_buff *skb,
> > gs_free_tx_context(txc);
> >
> > usb_unanchor_urb(urb);
> > - usb_free_coherent(dev->udev,
> > - sizeof(*hf), hf, urb->transfer_dma);
> > + usb_free_coherent(dev->udev, urb->transfer_buffer_length,
> > + urb->transfer_buffer, urb->transfer_dma);
> >
> > if (rc == -ENODEV) {
> > netif_device_detach(netdev);
> > @@ -552,7 +561,8 @@ static netdev_tx_t gs_can_start_xmit(struct sk_buff *skb,
> > return NETDEV_TX_OK;
> >
> > badidx:
> > - usb_free_coherent(dev->udev, sizeof(*hf), hf, urb->transfer_dma);
> > + usb_free_coherent(dev->udev, urb->transfer_buffer_length,
> > + urb->transfer_buffer, urb->transfer_dma);
> > nomem_hf:
> > usb_free_urb(urb);
> >
> > @@ -569,6 +579,7 @@ static int gs_can_open(struct net_device *netdev)
> > struct gs_usb *parent = dev->parent;
> > int rc, i;
> > struct gs_device_mode *dm;
> > + struct gs_host_frame *hf;
> > u32 ctrlmode;
> > u32 flags = 0;
> >
> > @@ -576,6 +587,8 @@ static int gs_can_open(struct net_device *netdev)
> > if (rc)
> > return rc;
> >
> > + dev->hf_size_tx = struct_size(hf, classic_can, 1);
>
> struct_size(hf, classic_can, 1) is a constant value, isn't it?
ACK
> Why not make this a macro?
It will not be constant with the addition of CAN-FD and the quirks
needed for some CAN devices.
> Also, what is the purpose of the FLEX_ARRAY if it contains only one element?
More will be added.
> I do not understand the logic. Sorry if I am wrong.
Granted, this patch without context looks a bit strange. Can we update
the patch description to make this easier to understand?
> > Modify struct gs_host_frame to make use of a union and
> > DECLARE_FLEX_ARRAY to be able to store different data (lengths), which
> > will be added in later commits.
> > +
> > if (!parent->active_channels) {
> > for (i = 0; i < GS_MAX_RX_URBS; i++) {
> > struct urb *urb;
> > @@ -588,7 +601,7 @@ static int gs_can_open(struct net_device *netdev)
> >
> > /* alloc rx buffer */
> > buf = usb_alloc_coherent(dev->udev,
> > - sizeof(struct gs_host_frame),
> > + dev->parent->hf_size_rx,
> > GFP_KERNEL,
> > &urb->transfer_dma);
> > if (!buf) {
> > @@ -604,7 +617,7 @@ static int gs_can_open(struct net_device *netdev)
> > usb_rcvbulkpipe(dev->udev,
> > GSUSB_ENDPOINT_IN),
> > buf,
> > - sizeof(struct gs_host_frame),
> > + dev->parent->hf_size_rx,
> > gs_usb_receive_bulk_callback, parent);
> > urb->transfer_flags |= URB_NO_TRANSFER_DMA_MAP;
> >
> > @@ -886,6 +899,7 @@ static int gs_usb_probe(struct usb_interface *intf,
> > const struct usb_device_id *id)
> > {
> > struct usb_device *udev = interface_to_usbdev(intf);
> > + struct gs_host_frame *hf;
> > struct gs_usb *dev;
> > int rc = -ENOMEM;
> > unsigned int icount, i;
> > @@ -947,6 +961,7 @@ static int gs_usb_probe(struct usb_interface *intf,
> > }
> >
> > init_usb_anchor(&dev->rx_submitted);
> > + dev->hf_size_rx = struct_size(hf, classic_can, 1);
>
> Same comment as hf_size_tx.
ACK, in a later patch they will be different. For TX we use the size
corresponding to the selected CAN mode (CAN-2.0 or CAN-FD) of the CAN
interface. For the RX direction we have to use CAN-FD if a at least one
CAN interface of the USB device uses CAN-FD.
The TX size is per CAN interface, the RX size if per USB device (which
can consist of several CAN interfaces).
Many thanks for the deep review, otherwise you wouldn't have found this!
regards,
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Embedded Linux | https://www.pengutronix.de |
Vertretung West/Dortmund | Phone: +49-231-2826-924 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
Attachment:
signature.asc
Description: PGP signature
