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?
Why not make this a macro?
Also, what is the purpose of the FLEX_ARRAY if it contains only one element?
I do not understand the logic. Sorry if I am wrong.
> +
> 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.
>
> usb_set_intfdata(intf, dev);
> dev->udev = udev;
> --
> 2.34.1
>
>
