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

[Vladimir Oltean]

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@xxxxxxx>
> > > > ---
> > >
> > > 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.