diff mbox series

[v2,08/18] can: m_can: Write transmit header and data in one transaction

Message ID 20230125195059.630377-9-msp@baylibre.com
State New
Headers
Series can: m_can: Optimizations for m_can/tcan part 2 |

Commit Message

Markus Schneider-Pargmann Jan. 25, 2023, 7:50 p.m. UTC 
  Combine header and data before writing to the transmit fifo to reduce
the overhead for peripheral chips.

Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com>
---
 drivers/net/can/m_can/m_can.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

Comments

Simon Horman Jan. 26, 2023, 8:04 a.m. UTC | #1 
On Wed, Jan 25, 2023 at 08:50:49PM +0100, Markus Schneider-Pargmann wrote:
> Combine header and data before writing to the transmit fifo to reduce
> the overhead for peripheral chips.
> 
> Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com>
> ---
>  drivers/net/can/m_can/m_can.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
> index 78f6ed744c36..440bc0536951 100644
> --- a/drivers/net/can/m_can/m_can.c
> +++ b/drivers/net/can/m_can/m_can.c
> @@ -1681,6 +1681,7 @@ static netdev_tx_t m_can_tx_handler(struct m_can_classdev *cdev)
>  		m_can_write(cdev, M_CAN_TXBAR, 0x1);
>  		/* End of xmit function for version 3.0.x */
>  	} else {
> +		char buf[TXB_ELEMENT_SIZE];
>  		/* Transmit routine for version >= v3.1.x */
>  
>  		txfqs = m_can_read(cdev, M_CAN_TXFQS);
> @@ -1720,12 +1721,11 @@ static netdev_tx_t m_can_tx_handler(struct m_can_classdev *cdev)
>  		fifo_header.dlc = FIELD_PREP(TX_BUF_MM_MASK, putidx) |
>  			FIELD_PREP(TX_BUF_DLC_MASK, can_fd_len2dlc(cf->len)) |
>  			fdflags | TX_BUF_EFC;
> -		err = m_can_fifo_write(cdev, putidx, M_CAN_FIFO_ID, &fifo_header, 2);
> -		if (err)
> -			goto out_fail;
> +		memcpy(buf, &fifo_header, 8);
> +		memcpy(&buf[8], &cf->data, cf->len);
>  
> -		err = m_can_fifo_write(cdev, putidx, M_CAN_FIFO_DATA,
> -				       cf->data, DIV_ROUND_UP(cf->len, 4));
> +		err = m_can_fifo_write(cdev, putidx, M_CAN_FIFO_ID,
> +				       buf, 8 + DIV_ROUND_UP(cf->len, 4));

Perhaps I am missing something here, but my reading is that:

- 8 is a length in bytes
- the 5th argument to m_can_fifo_write is the val_count parameter,
  whose unit is 4-byte long values.

  By this logic, perhaps the correct value for this argument is:

  DIV_ROUND_UP(8 + cf->len, 4)

Also:

- If cf->len is not a multiple of 4, is there a possibility
  that uninitialised trailing data in buf will be used
  indirectly by m_can_fifo_write()?

>  		if (err)
>  			goto out_fail;
>  
> -- 
> 2.39.0
>
Markus Schneider-Pargmann Jan. 30, 2023, 8:04 a.m. UTC | #2 
Hi Simon,

On Thu, Jan 26, 2023 at 09:04:56AM +0100, Simon Horman wrote:
> On Wed, Jan 25, 2023 at 08:50:49PM +0100, Markus Schneider-Pargmann wrote:
> > Combine header and data before writing to the transmit fifo to reduce
> > the overhead for peripheral chips.
> > 
> > Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com>
> > ---
> >  drivers/net/can/m_can/m_can.c | 10 +++++-----
> >  1 file changed, 5 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
> > index 78f6ed744c36..440bc0536951 100644
> > --- a/drivers/net/can/m_can/m_can.c
> > +++ b/drivers/net/can/m_can/m_can.c
> > @@ -1681,6 +1681,7 @@ static netdev_tx_t m_can_tx_handler(struct m_can_classdev *cdev)
> >  		m_can_write(cdev, M_CAN_TXBAR, 0x1);
> >  		/* End of xmit function for version 3.0.x */
> >  	} else {
> > +		char buf[TXB_ELEMENT_SIZE];
> >  		/* Transmit routine for version >= v3.1.x */
> >  
> >  		txfqs = m_can_read(cdev, M_CAN_TXFQS);
> > @@ -1720,12 +1721,11 @@ static netdev_tx_t m_can_tx_handler(struct m_can_classdev *cdev)
> >  		fifo_header.dlc = FIELD_PREP(TX_BUF_MM_MASK, putidx) |
> >  			FIELD_PREP(TX_BUF_DLC_MASK, can_fd_len2dlc(cf->len)) |
> >  			fdflags | TX_BUF_EFC;
> > -		err = m_can_fifo_write(cdev, putidx, M_CAN_FIFO_ID, &fifo_header, 2);
> > -		if (err)
> > -			goto out_fail;
> > +		memcpy(buf, &fifo_header, 8);
> > +		memcpy(&buf[8], &cf->data, cf->len);
> >  
> > -		err = m_can_fifo_write(cdev, putidx, M_CAN_FIFO_DATA,
> > -				       cf->data, DIV_ROUND_UP(cf->len, 4));
> > +		err = m_can_fifo_write(cdev, putidx, M_CAN_FIFO_ID,
> > +				       buf, 8 + DIV_ROUND_UP(cf->len, 4));
> 
> Perhaps I am missing something here, but my reading is that:
> 
> - 8 is a length in bytes
> - the 5th argument to m_can_fifo_write is the val_count parameter,
>   whose unit is 4-byte long values.
> 
>   By this logic, perhaps the correct value for this argument is:
> 
>   DIV_ROUND_UP(8 + cf->len, 4)

Thank you for spotting this. You are totally right, I will fix it for
the next version.

> 
> Also:
> 
> - If cf->len is not a multiple of 4, is there a possibility
>   that uninitialised trailing data in buf will be used
>   indirectly by m_can_fifo_write()?

Good point. I think this can only happen for 1, 2, 3, 5, 6, 7 bytes,
values above have to be multiple of 4 because of the CAN-FD
specification.

With 'buf' it should read garbage from the buffer which I think is not a
problem as the chip knows how much of the data to use. Also the tx
elemnt size is hardcoded to 64 byte in the driver, so we do not overwrite
the next element with that. The chip minimum size is 8 bytes for the
data field anyways. So I think this is fine.

Thanks,
Markus

> 
> >  		if (err)
> >  			goto out_fail;
> >  
> > -- 
> > 2.39.0
> >
Simon Horman Feb. 4, 2023, 1:05 p.m. UTC | #3 
On Mon, Jan 30, 2023 at 09:04:19AM +0100, Markus Schneider-Pargmann wrote:
> Hi Simon,
> 
> On Thu, Jan 26, 2023 at 09:04:56AM +0100, Simon Horman wrote:
> > On Wed, Jan 25, 2023 at 08:50:49PM +0100, Markus Schneider-Pargmann wrote:
> > > Combine header and data before writing to the transmit fifo to reduce
> > > the overhead for peripheral chips.
> > > 
> > > Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com>
> > > ---
> > >  drivers/net/can/m_can/m_can.c | 10 +++++-----
> > >  1 file changed, 5 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
> > > index 78f6ed744c36..440bc0536951 100644
> > > --- a/drivers/net/can/m_can/m_can.c
> > > +++ b/drivers/net/can/m_can/m_can.c
> > > @@ -1681,6 +1681,7 @@ static netdev_tx_t m_can_tx_handler(struct m_can_classdev *cdev)
> > >  		m_can_write(cdev, M_CAN_TXBAR, 0x1);
> > >  		/* End of xmit function for version 3.0.x */
> > >  	} else {
> > > +		char buf[TXB_ELEMENT_SIZE];
> > >  		/* Transmit routine for version >= v3.1.x */
> > >  
> > >  		txfqs = m_can_read(cdev, M_CAN_TXFQS);
> > > @@ -1720,12 +1721,11 @@ static netdev_tx_t m_can_tx_handler(struct m_can_classdev *cdev)
> > >  		fifo_header.dlc = FIELD_PREP(TX_BUF_MM_MASK, putidx) |
> > >  			FIELD_PREP(TX_BUF_DLC_MASK, can_fd_len2dlc(cf->len)) |
> > >  			fdflags | TX_BUF_EFC;
> > > -		err = m_can_fifo_write(cdev, putidx, M_CAN_FIFO_ID, &fifo_header, 2);
> > > -		if (err)
> > > -			goto out_fail;
> > > +		memcpy(buf, &fifo_header, 8);
> > > +		memcpy(&buf[8], &cf->data, cf->len);
> > >  
> > > -		err = m_can_fifo_write(cdev, putidx, M_CAN_FIFO_DATA,
> > > -				       cf->data, DIV_ROUND_UP(cf->len, 4));
> > > +		err = m_can_fifo_write(cdev, putidx, M_CAN_FIFO_ID,
> > > +				       buf, 8 + DIV_ROUND_UP(cf->len, 4));
> > 
> > Perhaps I am missing something here, but my reading is that:
> > 
> > - 8 is a length in bytes
> > - the 5th argument to m_can_fifo_write is the val_count parameter,
> >   whose unit is 4-byte long values.
> > 
> >   By this logic, perhaps the correct value for this argument is:
> > 
> >   DIV_ROUND_UP(8 + cf->len, 4)
> 
> Thank you for spotting this. You are totally right, I will fix it for
> the next version.

Thanks.

> > Also:
> > 
> > - If cf->len is not a multiple of 4, is there a possibility
> >   that uninitialised trailing data in buf will be used
> >   indirectly by m_can_fifo_write()?
> 
> Good point. I think this can only happen for 1, 2, 3, 5, 6, 7 bytes,
> values above have to be multiple of 4 because of the CAN-FD
> specification.
> 
> With 'buf' it should read garbage from the buffer which I think is not a
> problem as the chip knows how much of the data to use. Also the tx
> elemnt size is hardcoded to 64 byte in the driver, so we do not overwrite
> the next element with that. The chip minimum size is 8 bytes for the
> data field anyways. So I think this is fine.

I'm not the expert on the hw in question here, but intuitively
I do feel that it may be unwise to send uninitialised data.
While I'm happy to defer to you on this, I do wonder if it would be somehow
better to use memcpy_and_pad() in place of memcpy().
Markus Schneider-Pargmann Feb. 20, 2023, 5:31 a.m. UTC | #4 
Hi Simon,

On Sat, Feb 04, 2023 at 02:05:57PM +0100, Simon Horman wrote:
> On Mon, Jan 30, 2023 at 09:04:19AM +0100, Markus Schneider-Pargmann wrote:
> > Hi Simon,
> > 
> > On Thu, Jan 26, 2023 at 09:04:56AM +0100, Simon Horman wrote:
> > > On Wed, Jan 25, 2023 at 08:50:49PM +0100, Markus Schneider-Pargmann wrote:
> > > > Combine header and data before writing to the transmit fifo to reduce
> > > > the overhead for peripheral chips.
> > > > 
> > > > Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com>
> > > > ---
> > > >  drivers/net/can/m_can/m_can.c | 10 +++++-----
> > > >  1 file changed, 5 insertions(+), 5 deletions(-)
> > > > 
> > > > diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
> > > > index 78f6ed744c36..440bc0536951 100644
> > > > --- a/drivers/net/can/m_can/m_can.c
> > > > +++ b/drivers/net/can/m_can/m_can.c
> > > > @@ -1681,6 +1681,7 @@ static netdev_tx_t m_can_tx_handler(struct m_can_classdev *cdev)
> > > >  		m_can_write(cdev, M_CAN_TXBAR, 0x1);
> > > >  		/* End of xmit function for version 3.0.x */
> > > >  	} else {
> > > > +		char buf[TXB_ELEMENT_SIZE];
> > > >  		/* Transmit routine for version >= v3.1.x */
> > > >  
> > > >  		txfqs = m_can_read(cdev, M_CAN_TXFQS);
> > > > @@ -1720,12 +1721,11 @@ static netdev_tx_t m_can_tx_handler(struct m_can_classdev *cdev)
> > > >  		fifo_header.dlc = FIELD_PREP(TX_BUF_MM_MASK, putidx) |
> > > >  			FIELD_PREP(TX_BUF_DLC_MASK, can_fd_len2dlc(cf->len)) |
> > > >  			fdflags | TX_BUF_EFC;
> > > > -		err = m_can_fifo_write(cdev, putidx, M_CAN_FIFO_ID, &fifo_header, 2);
> > > > -		if (err)
> > > > -			goto out_fail;
> > > > +		memcpy(buf, &fifo_header, 8);
> > > > +		memcpy(&buf[8], &cf->data, cf->len);
> > > >  
> > > > -		err = m_can_fifo_write(cdev, putidx, M_CAN_FIFO_DATA,
> > > > -				       cf->data, DIV_ROUND_UP(cf->len, 4));
> > > > +		err = m_can_fifo_write(cdev, putidx, M_CAN_FIFO_ID,
> > > > +				       buf, 8 + DIV_ROUND_UP(cf->len, 4));
> > > 
> > > Perhaps I am missing something here, but my reading is that:
> > > 
> > > - 8 is a length in bytes
> > > - the 5th argument to m_can_fifo_write is the val_count parameter,
> > >   whose unit is 4-byte long values.
> > > 
> > >   By this logic, perhaps the correct value for this argument is:
> > > 
> > >   DIV_ROUND_UP(8 + cf->len, 4)
> > 
> > Thank you for spotting this. You are totally right, I will fix it for
> > the next version.
> 
> Thanks.
> 
> > > Also:
> > > 
> > > - If cf->len is not a multiple of 4, is there a possibility
> > >   that uninitialised trailing data in buf will be used
> > >   indirectly by m_can_fifo_write()?
> > 
> > Good point. I think this can only happen for 1, 2, 3, 5, 6, 7 bytes,
> > values above have to be multiple of 4 because of the CAN-FD
> > specification.
> > 
> > With 'buf' it should read garbage from the buffer which I think is not a
> > problem as the chip knows how much of the data to use. Also the tx
> > elemnt size is hardcoded to 64 byte in the driver, so we do not overwrite
> > the next element with that. The chip minimum size is 8 bytes for the
> > data field anyways. So I think this is fine.
> 
> I'm not the expert on the hw in question here, but intuitively
> I do feel that it may be unwise to send uninitialised data.
> While I'm happy to defer to you on this, I do wonder if it would be somehow
> better to use memcpy_and_pad() in place of memcpy().

Thank you, I think it is safe, but memcpy_and_pad seems like a good
solution here to make it even safer.

Best,
Markus
diff mbox series

Patch

diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
index 78f6ed744c36..440bc0536951 100644
--- a/drivers/net/can/m_can/m_can.c
+++ b/drivers/net/can/m_can/m_can.c
@@ -1681,6 +1681,7 @@  static netdev_tx_t m_can_tx_handler(struct m_can_classdev *cdev)
 		m_can_write(cdev, M_CAN_TXBAR, 0x1);
 		/* End of xmit function for version 3.0.x */
 	} else {
+		char buf[TXB_ELEMENT_SIZE];
 		/* Transmit routine for version >= v3.1.x */
 
 		txfqs = m_can_read(cdev, M_CAN_TXFQS);
@@ -1720,12 +1721,11 @@  static netdev_tx_t m_can_tx_handler(struct m_can_classdev *cdev)
 		fifo_header.dlc = FIELD_PREP(TX_BUF_MM_MASK, putidx) |
 			FIELD_PREP(TX_BUF_DLC_MASK, can_fd_len2dlc(cf->len)) |
 			fdflags | TX_BUF_EFC;
-		err = m_can_fifo_write(cdev, putidx, M_CAN_FIFO_ID, &fifo_header, 2);
-		if (err)
-			goto out_fail;
+		memcpy(buf, &fifo_header, 8);
+		memcpy(&buf[8], &cf->data, cf->len);
 
-		err = m_can_fifo_write(cdev, putidx, M_CAN_FIFO_DATA,
-				       cf->data, DIV_ROUND_UP(cf->len, 4));
+		err = m_can_fifo_write(cdev, putidx, M_CAN_FIFO_ID,
+				       buf, 8 + DIV_ROUND_UP(cf->len, 4));
 		if (err)
 			goto out_fail;