ethtool: ring configuration for CAN devices

ethtool: ring configuration for CAN devices

[Marc Kleine-Budde]

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

Hello,

I'm currently working on runtime configurable RX/TX ring sizes for a the
mcp251xfd CAN driver.

Unlike modern Ethernet cards with DMA support, most CAN IP cores come
with a fixed size on chip RAM that's used to store received CAN frames
and frames that should be sent.

For CAN-2.0 only devices that can be directly supported via ethtools's
set/get_ringparam. A minor unaesthetic is, as the on chip RAM is usually
shared between RX and TX, the maximum values for RX and TX cannot be set
at the same time.

The mcp251xfd chip I'm enhancing supports CAN-2.0 and CAN-FD mode. The
relevant difference of these modes is the size of the CAN frame. 8 vs 64
bytes of payload + 12 bytes of header. This means we have different
maximum values for both RX and TX for those modes.

How do we want to deal with the configuration of the two different
modes? As the current set/get_ringparam interface can configure the
mini- and jumbo frames for RX, but has only a single TX value.

Hao Chen and Guangbin Huang are laying the groundwork to extend the
ringparam interface via netlink:

| https://lore.kernel.org/all/20211014113943.16231-1-huangguangbin2@huawei.com

I was thinking about adding rx/tx_pending for CAN-FD. The use case would
be to configure the ring parameters independent of the current active
CAN mode. For example in systemd the RX/TX ring parameters are
configured in the .link file, while the CAN FD mode is configured in a
.network file. When switching to the other CAN mode, the previously
configured ring configuration of that CAN mode will be applied to the
hardware.

In my proof of concept implementation I'm misusing the struct
ethtool_ringparam's mini and jumbo values to pre-configure the CAN-2.0
and CAN-FD mode's RX ring size, but this is not mainlinable from my
point of view.

I'm interested in your opinion and use cases.

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

Re: ethtool: ring configuration for CAN devices

[Kurt Van Dijck]

On Sun, 24 Oct 2021 23:37:59 +0200, Marc Kleine-Budde wrote:
> Hello,
> 
> I'm currently working on runtime configurable RX/TX ring sizes for a the
> mcp251xfd CAN driver.
> 
> Unlike modern Ethernet cards with DMA support, most CAN IP cores come
> with a fixed size on chip RAM that's used to store received CAN frames
> and frames that should be sent.
> 
> For CAN-2.0 only devices that can be directly supported via ethtools's
> set/get_ringparam. A minor unaesthetic is, as the on chip RAM is usually
> shared between RX and TX, the maximum values for RX and TX cannot be set
> at the same time.
> 
> The mcp251xfd chip I'm enhancing supports CAN-2.0 and CAN-FD mode. The
> relevant difference of these modes is the size of the CAN frame. 8 vs 64
> bytes of payload + 12 bytes of header. This means we have different
> maximum values for both RX and TX for those modes.
> 
> How do we want to deal with the configuration of the two different
> modes? As the current set/get_ringparam interface can configure the
> mini- and jumbo frames for RX, but has only a single TX value.
> 
> Hao Chen and Guangbin Huang are laying the groundwork to extend the
> ringparam interface via netlink:
> 
> | https://lore.kernel.org/all/20211014113943.16231-1-huangguangbin2@huawei.com
> 
> I was thinking about adding rx/tx_pending for CAN-FD. The use case would
> be to configure the ring parameters independent of the current active
> CAN mode. For example in systemd the RX/TX ring parameters are
> configured in the .link file, while the CAN FD mode is configured in a
> .network file. When switching to the other CAN mode, the previously
> configured ring configuration of that CAN mode will be applied to the
> hardware.
> 
> In my proof of concept implementation I'm misusing the struct
> ethtool_ringparam's mini and jumbo values to pre-configure the CAN-2.0
> and CAN-FD mode's RX ring size, but this is not mainlinable from my
> point of view.
> 
> I'm interested in your opinion and use cases.

Isn't the simplest setup to stick to the current CAN mode (2.0 vs. FD).

Certain values/combinations may be valid in 2.0 and not in FD. So what?
This is true also for data-bittiming and what not.

I see no advantage in putting your configuration in different files
(.link and .network), since they influence each other.
I can imaging one network operating in FD, with certain rx/tx settings,
and another network operating in 2.0, with different rx/tx settings.
and a 3rd network operating in FD, with also different rx/tx settings.

If that is a problem for systemd, then ... fix systemd?
(systemd is really out of my scope, I'm not used to it)

IMHO, you try to provide different default settings (rx/tx split) for FD
and 2.0 mode.

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

Re: ethtool: ring configuration for CAN devices

[Marc Kleine-Budde]

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

On 25.10.2021 11:00:08, Kurt Van Dijck wrote:
> On Sun, 24 Oct 2021 23:37:59 +0200, Marc Kleine-Budde wrote:
> > Hello,
> > 
> > I'm currently working on runtime configurable RX/TX ring sizes for a the
> > mcp251xfd CAN driver.
> > 
> > Unlike modern Ethernet cards with DMA support, most CAN IP cores come
> > with a fixed size on chip RAM that's used to store received CAN frames
> > and frames that should be sent.
> > 
> > For CAN-2.0 only devices that can be directly supported via ethtools's
> > set/get_ringparam. A minor unaesthetic is, as the on chip RAM is usually
> > shared between RX and TX, the maximum values for RX and TX cannot be set
> > at the same time.
> > 
> > The mcp251xfd chip I'm enhancing supports CAN-2.0 and CAN-FD mode. The
> > relevant difference of these modes is the size of the CAN frame. 8 vs 64
> > bytes of payload + 12 bytes of header. This means we have different
> > maximum values for both RX and TX for those modes.
> > 
> > How do we want to deal with the configuration of the two different
> > modes? As the current set/get_ringparam interface can configure the
> > mini- and jumbo frames for RX, but has only a single TX value.
> > 
> > Hao Chen and Guangbin Huang are laying the groundwork to extend the
> > ringparam interface via netlink:
> > 
> > | https://lore.kernel.org/all/20211014113943.16231-1-huangguangbin2@huawei.com
> > 
> > I was thinking about adding rx/tx_pending for CAN-FD. The use case would
> > be to configure the ring parameters independent of the current active
> > CAN mode. For example in systemd the RX/TX ring parameters are
> > configured in the .link file, while the CAN FD mode is configured in a
> > .network file. When switching to the other CAN mode, the previously
> > configured ring configuration of that CAN mode will be applied to the
> > hardware.
> > 
> > In my proof of concept implementation I'm misusing the struct
> > ethtool_ringparam's mini and jumbo values to pre-configure the CAN-2.0
> > and CAN-FD mode's RX ring size, but this is not mainlinable from my
> > point of view.
> > 
> > I'm interested in your opinion and use cases.
> 
> Isn't the simplest setup to stick to the current CAN mode (2.0 vs. FD).
> 
> Certain values/combinations may be valid in 2.0 and not in FD. So what?
> This is true also for data-bittiming and what not.
> 
> I see no advantage in putting your configuration in different files
> (.link and .network), since they influence each other.

That's the way systemd has chosen to put these values....

> I can imaging one network operating in FD, with certain rx/tx settings,
> and another network operating in 2.0, with different rx/tx settings.
> and a 3rd network operating in FD, with also different rx/tx settings.

ACK - the .link and .network files are per network interface, so that
should be no problem to have different settings for different CAN
interfaces.

> If that is a problem for systemd, then ... fix systemd?
> (systemd is really out of my scope, I'm not used to it)
> 
> IMHO, you try to provide different default settings (rx/tx split) for FD
> and 2.0 mode.

ACK - the driver provides default settings for CAN-2.0 and CAN-FD mode
and I want to overwrite both default settings via ethtool ring settings.
So that these overwritten settings are used when switching from CAN to
CAN-FD mode an back.

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

Re: ethtool: ring configuration for CAN devices

[Andrew Lunn]

On Sun, Oct 24, 2021 at 11:37:59PM +0200, Marc Kleine-Budde wrote:
> Hello,
> 
> I'm currently working on runtime configurable RX/TX ring sizes for a the
> mcp251xfd CAN driver.
> 
> Unlike modern Ethernet cards with DMA support, most CAN IP cores come
> with a fixed size on chip RAM that's used to store received CAN frames
> and frames that should be sent.
> 
> For CAN-2.0 only devices that can be directly supported via ethtools's
> set/get_ringparam. A minor unaesthetic is, as the on chip RAM is usually
> shared between RX and TX, the maximum values for RX and TX cannot be set
> at the same time.
> 
> The mcp251xfd chip I'm enhancing supports CAN-2.0 and CAN-FD mode. The
> relevant difference of these modes is the size of the CAN frame. 8 vs 64
> bytes of payload + 12 bytes of header. This means we have different
> maximum values for both RX and TX for those modes.
> 
> How do we want to deal with the configuration of the two different
> modes? As the current set/get_ringparam interface can configure the
> mini- and jumbo frames for RX, but has only a single TX value.

Hi Marc

I would not consider it as two different modes, but as N modes. That
way, we are prepared for CAN-3.0 which might need other ring
parameters.

The netlink API is extensible, unlike the IOCTL interface. I would add
an additional optional attribute, ETHTOOL_A_RINGS_MODE, with values like:

ETHTOOL_A_RINGS_MODE_DEFAULT
ETHTOOL_A_RINGS_MODE_CAN_2
ETHTOOL_A_RINGS_MODE_CAN_FD

The IOCTL would always be for mode _DEFAULT, and it would get/set the
current used setting. If the optionally attribute is missing, then the
calling into the driver would also use _DEFAULT. However, if it is
present, the driver can store away the ring parameters for a
particular mode, and maybe actually put them into use if the mode is
currently active.

You cannot change

struct ethtool_ringparam {
	__u32	cmd;
	__u32	rx_max_pending;
	__u32	rx_mini_max_pending;
	__u32	rx_jumbo_max_pending;
	__u32	tx_max_pending;
	__u32	rx_pending;
	__u32	rx_mini_pending;
	__u32	rx_jumbo_pending;
	__u32	tx_pending;
};

Since that is ABI. But you can add an

struct ethtool_kringparam {
	__u32	cmd;
	__u32   mode;
	__u32	rx_max_pending;
	__u32	rx_mini_max_pending;
	__u32	rx_jumbo_max_pending;
	__u32	tx_max_pending;
	__u32	rx_pending;
	__u32	rx_mini_pending;
	__u32	rx_jumbo_pending;
	__u32	tx_pending;
};

and use this structure between the ethtool core and the drivers. This
has already been done at least once to allow extending the
API. Semantic patches are good for making the needed changes to all
the drivers.

     Andrew

Re: ethtool: ring configuration for CAN devices

[Marc Kleine-Budde]

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

On 25.10.2021 14:27:06, Andrew Lunn wrote:
> I would not consider it as two different modes, but as N modes. That
> way, we are prepared for CAN-3.0 which might need other ring
> parameters.

ACK - there's CAN-XL, but I haven't seen any HW, yet. Let's follow the
zero one infinity rule :)

> The netlink API is extensible, unlike the IOCTL interface. I would add
> an additional optional attribute, ETHTOOL_A_RINGS_MODE, with values like:
> 
> ETHTOOL_A_RINGS_MODE_DEFAULT
> ETHTOOL_A_RINGS_MODE_CAN_2
> ETHTOOL_A_RINGS_MODE_CAN_FD
> 
> The IOCTL would always be for mode _DEFAULT, and it would get/set the
> current used setting. If the optionally attribute is missing, then the
> calling into the driver would also use _DEFAULT. However, if it is
> present, the driver can store away the ring parameters for a
> particular mode, and maybe actually put them into use if the mode is
> currently active.
> 
> You cannot change
> 
> struct ethtool_ringparam {
> 	__u32	cmd;
> 	__u32	rx_max_pending;
> 	__u32	rx_mini_max_pending;
> 	__u32	rx_jumbo_max_pending;
> 	__u32	tx_max_pending;
> 	__u32	rx_pending;
> 	__u32	rx_mini_pending;
> 	__u32	rx_jumbo_pending;
> 	__u32	tx_pending;
> };
> 
> Since that is ABI.

ACK

> But you can add an
> 
> struct ethtool_kringparam {
> 	__u32	cmd;
> 	__u32   mode;
> 	__u32	rx_max_pending;
> 	__u32	rx_mini_max_pending;
> 	__u32	rx_jumbo_max_pending;
> 	__u32	tx_max_pending;
> 	__u32	rx_pending;
> 	__u32	rx_mini_pending;
> 	__u32	rx_jumbo_pending;
> 	__u32	tx_pending;
> };
> 
> and use this structure between the ethtool core and the drivers. This
> has already been done at least once to allow extending the
> API. Semantic patches are good for making the needed changes to all
> the drivers.

What about the proposed "two new parameters ringparam_ext and extack for
.get_ringparam and .set_ringparam to extend more ring params through
netlink." by Hao Chen/Guangbin Huang in:

https://lore.kernel.org/all/20211014113943.16231-5-huangguangbin2@huawei.com/

I personally like the conversion of the in in-kernel API to struct
ethtool_kringparam better than adding ringparam_ext.


Thanks for you input, 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 --]

Re: ethtool: ring configuration for CAN devices

[Andrew Lunn]

> > struct ethtool_kringparam {
> > 	__u32	cmd;
> > 	__u32   mode;
> > 	__u32	rx_max_pending;
> > 	__u32	rx_mini_max_pending;
> > 	__u32	rx_jumbo_max_pending;
> > 	__u32	tx_max_pending;
> > 	__u32	rx_pending;
> > 	__u32	rx_mini_pending;
> > 	__u32	rx_jumbo_pending;
> > 	__u32	tx_pending;
> > };
> > 
> > and use this structure between the ethtool core and the drivers. This
> > has already been done at least once to allow extending the
> > API. Semantic patches are good for making the needed changes to all
> > the drivers.
> 
> What about the proposed "two new parameters ringparam_ext and extack for
> .get_ringparam and .set_ringparam to extend more ring params through
> netlink." by Hao Chen/Guangbin Huang in:
> 
> https://lore.kernel.org/all/20211014113943.16231-5-huangguangbin2@huawei.com/
>
> I personally like the conversion of the in in-kernel API to struct
> ethtool_kringparam better than adding ringparam_ext.

Ah, i missed that development. I don't like it.

You should probably jump into that discussion and explain your
requirements. Make sure it is heading in a direction you can extend
for your needs.

    Andrew

Re: ethtool: ring configuration for CAN devices

[Marc Kleine-Budde]

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

On 25.10.2021 14:58:58, Andrew Lunn wrote:
> > > struct ethtool_kringparam {
> > > 	__u32	cmd;
> > > 	__u32   mode;
> > > 	__u32	rx_max_pending;
> > > 	__u32	rx_mini_max_pending;
> > > 	__u32	rx_jumbo_max_pending;
> > > 	__u32	tx_max_pending;
> > > 	__u32	rx_pending;
> > > 	__u32	rx_mini_pending;
> > > 	__u32	rx_jumbo_pending;
> > > 	__u32	tx_pending;
> > > };
> > > 
> > > and use this structure between the ethtool core and the drivers. This
> > > has already been done at least once to allow extending the
> > > API. Semantic patches are good for making the needed changes to all
> > > the drivers.
> > 
> > What about the proposed "two new parameters ringparam_ext and extack for
> > .get_ringparam and .set_ringparam to extend more ring params through
> > netlink." by Hao Chen/Guangbin Huang in:
> > 
> > https://lore.kernel.org/all/20211014113943.16231-5-huangguangbin2@huawei.com/
> >
> > I personally like the conversion of the in in-kernel API to struct
> > ethtool_kringparam better than adding ringparam_ext.
> 
> Ah, i missed that development. I don't like it.
> 
> You should probably jump into that discussion and explain your
> requirements. Make sure it is heading in a direction you can extend
> for your needs.

Will do. I've added you on Cc.

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

Re: ethtool: ring configuration for CAN devices

[Jakub Kicinski]

On Mon, 25 Oct 2021 14:58:58 +0200 Andrew Lunn wrote:
> > > struct ethtool_kringparam {
> > > 	__u32	cmd;
> > > 	__u32   mode;
> > > 	__u32	rx_max_pending;
> > > 	__u32	rx_mini_max_pending;
> > > 	__u32	rx_jumbo_max_pending;
> > > 	__u32	tx_max_pending;
> > > 	__u32	rx_pending;
> > > 	__u32	rx_mini_pending;
> > > 	__u32	rx_jumbo_pending;
> > > 	__u32	tx_pending;
> > > };

Ah, yes, if we do full field-by-field translation the result is not as
bad as embedding the "base" structure, at the cost of additional
boilerplate code in the core. But frankly potato, potatoe.

> > > and use this structure between the ethtool core and the drivers. This
> > > has already been done at least once to allow extending the
> > > API. Semantic patches are good for making the needed changes to all
> > > the drivers.  
> > 
> > What about the proposed "two new parameters ringparam_ext and extack for
> > .get_ringparam and .set_ringparam to extend more ring params through
> > netlink." by Hao Chen/Guangbin Huang in:
> > 
> > https://lore.kernel.org/all/20211014113943.16231-5-huangguangbin2@huawei.com/
> >
> > I personally like the conversion of the in in-kernel API to struct
> > ethtool_kringparam better than adding ringparam_ext.  
> 
> Ah, i missed that development.

I think it's the fifth version and people are starting to have feedback.
Something is not working :(

> I don't like it.
> 
> You should probably jump into that discussion and explain your
> requirements. Make sure it is heading in a direction you can extend
> for your needs.