Re: [PATCH net-next v2 03/11] net: dsa: microchip: split ksz_common.h

Vladimir Oltean
2020-11-16T10:47:01Z

On Mon, Nov 16, 2020 at 10:21:14AM +0100, Christian Eggers wrote:
> On Friday, 13 November 2020, 17:56:34 CET, Christian Eggers wrote:
> > On Friday, 13 November 2020, 00:02:54 CET, Vladimir Oltean wrote:
> > > On Thu, Nov 12, 2020 at 04:35:29PM +0100, Christian Eggers wrote:
> > > > Parts of ksz_common.h (struct ksz_device) will be required in
> > > > net/dsa/tag_ksz.c soon. So move the relevant parts into a new header
> > > > file.
> > > >
> > > > Signed-off-by: Christian Eggers <ceggers@arri.de>
> > > > ---
> > >
> > > I had to skip ahead to see what you're going to use struct ksz_port and
> > >
> > > struct ksz_device for. It looks like you need:
> > > struct ksz_port::tstamp_rx_latency_ns
> > > struct ksz_device::ptp_clock_lock
> > > struct ksz_device::ptp_clock_time
> > >
> > > Not more.
> I have tried to put these members into separate structs:
>
> include/linux/dsa/ksz_common.h:
> struct ksz_port_ptp_shared {
> u16 tstamp_rx_latency_ns; /* rx delay from wire to tstamp unit */
> };
>
> struct ksz_device_ptp_shared {
> spinlock_t ptp_clock_lock; /* for ptp_clock_time */
> /* approximated current time, read once per second from hardware */
> struct timespec64 ptp_clock_time;
> };
>
> drivers/net/dsa/microchip/ksz_common.h:
> ...
> #include <linux/dsa/ksz_common.h>
> ...
> struct ksz_port {
> ...
> #if IS_ENABLED(CONFIG_NET_DSA_MICROCHIP_KSZ9477_PTP)
> struct ksz_port_ptp_shared ptp_shared; /* shared with tag_ksz.c */
> u16 tstamp_tx_latency_ns; /* tx delay from tstamp unit to wire */
> struct hwtstamp_config tstamp_config;
> struct sk_buff *tstamp_tx_xdelay_skb;
> unsigned long tstamp_state;
> #endif
> };
> ...
> struct ksz_device {
> ...
> #if IS_ENABLED(CONFIG_NET_DSA_MICROCHIP_KSZ9477_PTP)
> struct ptp_clock *ptp_clock;
> struct ptp_clock_info ptp_caps;
> struct mutex ptp_mutex;
> struct ksz_device_ptp_shared ptp_shared; /* shared with tag_ksz.c */
> #endif
> };
>
> The problem with such technique is, that I still need to dereference
> struct ksz_device in tag_ksz.c:
>
> static void ksz9477_rcv_timestamp(struct sk_buff *skb, u8 *tag,
> struct net_device *dev, unsigned int port)
> {
> ...
> struct dsa_switch *ds = dev->dsa_ptr->ds;
> struct ksz_device *ksz = ds->priv;
> struct ksz_port *prt = &ksz->ports[port];
> ...
> }
>
> As struct dsa_switch::priv is already occupied by the pointer to
> struct ksz_device, I see no way accessing the ptp specific device/port
> information in tag_ksz.c.

There is a dp->priv that you could use to hold a reference to your
PTP-specific substructure (struct ksz_port_ptp_shared) of
struct ksz_port.

Then, in that PTP-specific per-port substructure, you could hold another
pointer to a common struct ksz_device_ptp_shared.