netdev - Re: [PATCH net-next v2 03/11] net: dsa: microchip: split ksz

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 16 Nov 2020 10:21:14 +0100
From:   Christian Eggers <ceggers@...i.de>
To:     Vladimir Oltean <olteanv@...il.com>
CC:     Jakub Kicinski <kuba@...nel.org>, Andrew Lunn <andrew@...n.ch>,
        "Richard Cochran" <richardcochran@...il.com>,
        Rob Herring <robh+dt@...nel.org>,
        "Vivien Didelot" <vivien.didelot@...il.com>,
        "David S . Miller" <davem@...emloft.net>,
        Kurt Kanzenbach <kurt.kanzenbach@...utronix.de>,
        George McCollister <george.mccollister@...il.com>,
        Marek Vasut <marex@...x.de>,
        Helmut Grohne <helmut.grohne@...enta.de>,
        Paul Barker <pbarker@...sulko.com>,
        "Codrin Ciubotariu" <codrin.ciubotariu@...rochip.com>,
        Tristram Ha <Tristram.Ha@...rochip.com>,
        Woojung Huh <woojung.huh@...rochip.com>,
        Microchip Linux Driver Support <UNGLinuxDriver@...rochip.com>,
        <netdev@...r.kernel.org>, <devicetree@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH net-next v2 03/11] net: dsa: microchip: split ksz_common.h

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

> > 
> > Why don't you go the other way around, i.e. exporting some functions
> > from your driver, and calling them from the tagger?
> 
> Good question... But as for as I can see, there are a single tagger and
> multiple device drivers (currently KSZ8795 and KSZ9477).
> 
> Moving the KSZ9477 specific stuff, which is required by the tagger, into the
> KSZ9477 device driver, would make the tagger dependent on the driver(s).
> Currently, no tagger seems to have this direction of dependency (at least I
> cannot find this in net/dsa/Kconfig).
> 
> If I shall change this anyway, I would use #ifdefs within the tag_ksz driver
> in order to avoid unnecessary dependencies to the KSZ9477 driver for the
> case only KSZ8795 is selected.
> 
> > You could even move
> > the entire ksz9477_tstamp_to_clock() into the driver as-is, as far as I
> > can see.

regards
Christian