| Message ID | 20200820081118.10105-1-kurt@linutronix.de |
|---|---|
| Headers | show |
| Series | Hirschmann Hellcreek DSA driver | expand |
On Thu, 20 Aug 2020 10:11:10 +0200 Kurt Kanzenbach wrote: > this series adds a DSA driver for the Hirschmann Hellcreek TSN switch > IP. Characteristics of that IP: > > * Full duplex Ethernet interface at 100/1000 Mbps on three ports > * IEEE 802.1Q-compliant Ethernet Switch > * IEEE 802.1Qbv Time-Aware scheduling support > * IEEE 1588 and IEEE 802.1AS support I don't see anything worth complaining about here, but this is not my area of expertise.. DSA and TAPRIO folks - does this look good to you?
On Mon, Aug 24, 2020 at 02:31:10PM -0700, Jakub Kicinski wrote: > On Thu, 20 Aug 2020 10:11:10 +0200 Kurt Kanzenbach wrote: > > this series adds a DSA driver for the Hirschmann Hellcreek TSN switch > > IP. Characteristics of that IP: > > > > * Full duplex Ethernet interface at 100/1000 Mbps on three ports > > * IEEE 802.1Q-compliant Ethernet Switch > > * IEEE 802.1Qbv Time-Aware scheduling support > > * IEEE 1588 and IEEE 802.1AS support > > I don't see anything worth complaining about here, but this is not my > area of expertise.. > > DSA and TAPRIO folks - does this look good to you? Just my comment on patch 5/8 about netdev->tc_to_txq. There are 2 distinct things about that: - accessing struct net_device directly hurts the DSA model a little bit. - I think there's some confusion regarding the use of netdev->tc_to_txq itself. I don't think that's the right place to setup a VLAN PCP to traffic class mapping. That's simply "what traffic class does each netdev queue have". I would even go as far as say that Linux doesn't support a VLAN PCP to TC mapping (similar to the DSCP to TC mapping from the DCB ops) at all, except for the ingress-qos-map and egress-qos-map of the 8021q driver, which can't be offloaded and don't map nicely over existing hardware anyway (what hardware has an ingress-qos-map and an egress-qos-map per individual VLAN?!). Although I do really see the need for having a mapping between VLAN PCP and traffic class, I would suggest Kurt to not expose this through taprio/mqprio (hardcode the PCP mapping as 1-to-1 with TC, as other drivers do), and let's try to come up separately with an abstraction for that. Thanks, -Vladimir
From: Vladimir Oltean <olteanv@gmail.com> Date: Tue, 25 Aug 2020 01:02:03 +0300 > Just my comment on patch 5/8 about netdev->tc_to_txq. There are 2 > distinct things about that: > - accessing struct net_device directly hurts the DSA model a little bit. > - I think there's some confusion regarding the use of netdev->tc_to_txq > itself. I don't think that's the right place to setup a VLAN PCP to > traffic class mapping. That's simply "what traffic class does each > netdev queue have". I would even go as far as say that Linux doesn't > support a VLAN PCP to TC mapping (similar to the DSCP to TC mapping > from the DCB ops) at all, except for the ingress-qos-map and > egress-qos-map of the 8021q driver, which can't be offloaded and don't > map nicely over existing hardware anyway (what hardware has an > ingress-qos-map and an egress-qos-map per individual VLAN?!). > Although I do really see the need for having a mapping between VLAN > PCP and traffic class, I would suggest Kurt to not expose this through > taprio/mqprio (hardcode the PCP mapping as 1-to-1 with TC, as other > drivers do), and let's try to come up separately with an abstraction > for that. Agreed, Kurt can you repost this series without the TAPRIO support for now since it's controversial and needs more discussion and changes? Thank you.
On Mon, Aug 24, 2020 at 03:35:18PM -0700, David Miller wrote: > From: Vladimir Oltean <olteanv@gmail.com> > Date: Tue, 25 Aug 2020 01:02:03 +0300 > > > Just my comment on patch 5/8 about netdev->tc_to_txq. There are 2 > > distinct things about that: > > - accessing struct net_device directly hurts the DSA model a little bit. > > - I think there's some confusion regarding the use of netdev->tc_to_txq > > itself. I don't think that's the right place to setup a VLAN PCP to > > traffic class mapping. That's simply "what traffic class does each > > netdev queue have". I would even go as far as say that Linux doesn't > > support a VLAN PCP to TC mapping (similar to the DSCP to TC mapping > > from the DCB ops) at all, except for the ingress-qos-map and > > egress-qos-map of the 8021q driver, which can't be offloaded and don't > > map nicely over existing hardware anyway (what hardware has an > > ingress-qos-map and an egress-qos-map per individual VLAN?!). > > Although I do really see the need for having a mapping between VLAN > > PCP and traffic class, I would suggest Kurt to not expose this through > > taprio/mqprio (hardcode the PCP mapping as 1-to-1 with TC, as other > > drivers do), and let's try to come up separately with an abstraction > > for that. > > Agreed, Kurt can you repost this series without the TAPRIO support for > now since it's controversial and needs more discussion and changes? > > Thank you. To be clear, the most important part of the taprio qdisc offload (setting up the gate control list) does not need to be postponed. It's just the VLAN PCP mapping that is a bit controversial. Thanks, -Vladimir
On Mon Aug 24 2020, David Miller wrote: > Agreed, Kurt can you repost this series without the TAPRIO support for > now since it's controversial and needs more discussion and changes? OK. It seems like the TAPRIO implementation has to be discussed more and it might be good to do that separately. I'll replace the spinlocks (which were only introduced for the hrtimers) with mutexes and post a sane version of the driver without the TAPRIO support. > > Thank you. Thanks, Kurt
On 8/25/2020 4:21 AM, Kurt Kanzenbach wrote: > On Mon Aug 24 2020, David Miller wrote: >> Agreed, Kurt can you repost this series without the TAPRIO support for >> now since it's controversial and needs more discussion and changes? > > OK. It seems like the TAPRIO implementation has to be discussed more and > it might be good to do that separately. > > I'll replace the spinlocks (which were only introduced for the hrtimers) > with mutexes and post a sane version of the driver without the TAPRIO > support. Sounds great, thanks!