All of lore.kernel.org
 help / color / mirror / Atom feed
From: Marc Kleine-Budde <mkl@pengutronix.de>
To: Vincent Mailhol <vincent.mailhol@gmail.com>
Cc: linux-can@vger.kernel.org, kernel@pengutronix.de,
	Peter Fink <pfink@christ-es.de>
Subject: Re: [can-next-rfc 14/21] can: gs_usb: use union and FLEX_ARRAY for data in struct gs_host_frame
Date: Wed, 9 Mar 2022 15:18:29 +0100	[thread overview]
Message-ID: <20220309141829.33glce2cy4ylvzzg@pengutronix.de> (raw)
In-Reply-To: <CAMZ6Rq+R-yxYm4Kk+aoaQXNedKkmq0LbwDSxs0nXdJMn6t+=bw@mail.gmail.com>

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

On 09.03.2022 23:05:57, Vincent Mailhol wrote:
> On Wed. 9 Mar 2022 à 22:39, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> > From: Peter Fink <pfink@christ-es.de>
> >
> > 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@christ-es.de>
> > Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
> > ---
> >  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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

  parent reply	other threads:[~2022-03-09 14:18 UTC|newest]

Thread overview: 26+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-03-09 12:41 [RFC]: can-next gs-usb Marc Kleine-Budde
2022-03-09 12:41 ` [can-next-rfc 01/21] can: gs_usb: use consistent one space indention Marc Kleine-Budde
2022-03-09 12:41 ` [can-next-rfc 02/21] can: gs_usb: fix checkpatch warning Marc Kleine-Budde
2022-03-09 12:41 ` [can-next-rfc 03/21] can: gs_usb: sort include files alphabetically Marc Kleine-Budde
2022-03-09 12:41 ` [can-next-rfc 04/21] can: gs_usb: GS_CAN_FLAG_OVERFLOW: make use of BIT() Marc Kleine-Budde
2022-03-09 12:41 ` [can-next-rfc 05/21] can: gs_usb: rewrap error messages Marc Kleine-Budde
2022-03-09 12:41 ` [can-next-rfc 06/21] can: gs_usb: rewrap usb_control_msg() and usb_fill_bulk_urb() Marc Kleine-Budde
2022-03-09 12:41 ` [can-next-rfc 07/21] can: gs_usb: gs_make_candev(): call SET_NETDEV_DEV() after handling all bt_const->feature Marc Kleine-Budde
2022-03-09 12:41 ` [can-next-rfc 08/21] can: gs_usb: add HW timestamp mode bit Marc Kleine-Budde
2022-03-09 12:41 ` [can-next-rfc 09/21] can: gs_usb: update GS_CAN_FEATURE_IDENTIFY documentation Marc Kleine-Budde
2022-03-09 12:41 ` [can-next-rfc 10/21] can: gs_usb: document the USER_ID feature Marc Kleine-Budde
2022-03-09 12:41 ` [can-next-rfc 11/21] can: gs_usb: document the PAD_PKTS_TO_MAX_PKT_SIZE feature Marc Kleine-Budde
2022-03-09 12:41 ` [can-next-rfc 12/21] can: gs_usb: gs_usb_probe(): introduce udev and make use of it Marc Kleine-Budde
2022-03-09 12:41 ` [can-next-rfc 13/21] can: gs_usb: support up to 3 channels per device Marc Kleine-Budde
2022-03-09 12:41 ` [can-next-rfc 14/21] can: gs_usb: use union and FLEX_ARRAY for data in struct gs_host_frame Marc Kleine-Budde
2022-03-09 14:05   ` Vincent Mailhol
2022-03-09 14:16     ` Vincent Mailhol
2022-03-09 14:20       ` Marc Kleine-Budde
2022-03-09 14:18     ` Marc Kleine-Budde [this message]
2022-03-09 12:41 ` [can-next-rfc 15/21] can: gs_usb: add CAN-FD support Marc Kleine-Budde
2022-03-09 12:41 ` [can-next-rfc 16/21] can: gs_usb: add usb quirk for NXP LPC546xx controllers Marc Kleine-Budde
2022-03-09 12:41 ` [can-next-rfc 17/21] can: gs_usb: add quirk for CANtact Pro overlapping GS_USB_BREQ value Marc Kleine-Budde
2022-03-09 12:41 ` [can-next-rfc 18/21] can: gs_usb: activate quirks for CANtact Pro unconditionally Marc Kleine-Budde
2022-03-09 12:41 ` [can-next-rfc 19/21] can: gs_usb: add extended bt_const feature Marc Kleine-Budde
2022-03-09 12:41 ` [can-next-rfc 20/21] can: gs_usb: add VID/PID for CES CANext FD devices Marc Kleine-Budde
2022-03-09 12:41 ` [can-next-rfc 21/21] can: gs_usb: add VID/PID for ABE CAN Debugger devices Marc Kleine-Budde

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20220309141829.33glce2cy4ylvzzg@pengutronix.de \
    --to=mkl@pengutronix.de \
    --cc=kernel@pengutronix.de \
    --cc=linux-can@vger.kernel.org \
    --cc=pfink@christ-es.de \
    --cc=vincent.mailhol@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.