netdev - Re: [PATCH 11/14] net: ks8851: Implement register and FIFO accessor callbacks

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]

Message-ID: <20200324142929.dfs2cdmj3na27oo5@wunner.de>
Date:   Tue, 24 Mar 2020 15:29:29 +0100
From:   Lukas Wunner <lukas@...ner.de>
To:     Marek Vasut <marex@...x.de>
Cc:     netdev@...r.kernel.org, "David S . Miller" <davem@...emloft.net>,
        Petr Stetiar <ynezz@...e.cz>,
        YueHaibing <yuehaibing@...wei.com>, Andrew Lunn <andrew@...n.ch>
Subject: Re: [PATCH 11/14] net: ks8851: Implement register and FIFO accessor
 callbacks

On Tue, Mar 24, 2020 at 03:10:59PM +0100, Marek Vasut wrote:
> On 3/24/20 2:45 PM, Lukas Wunner wrote:
> > On Tue, Mar 24, 2020 at 12:43:00AM +0100, Marek Vasut wrote:
> >> The register and FIFO accessors are bus specific. Implement callbacks so
> >> that each variant of the KS8851 can implement matching accessors and use
> >> the rest of the common code.
> > [...]
> >> +	unsigned int		(*rdreg16)(struct ks8851_net *ks,
> >> +					   unsigned int reg);
> >> +	void			(*wrreg16)(struct ks8851_net *ks,
> >> +					   unsigned int reg, unsigned int val);
> >> +	void			(*rdfifo)(struct ks8851_net *ks, u8 *buff,
> >> +					  unsigned int len);
> >> +	void			(*wrfifo)(struct ks8851_net *ks,
> >> +					  struct sk_buff *txp, bool irq);
> > 
> > Using callbacks entails a dereference for each invocation.
> 
> Yes indeed, the SPI stack which you use to talk to the KS8851 SPI is
> also full of those.

Apples and oranges.  Low-level SPI drivers provide callbacks to the
SPI core because it would be too expensive (space-wise) to link the
SPI core into every low-level driver.  Whereas in this case, you're
generating two separate modules anyway, so there's no need at all
to use callbacks.


> > A cheaper approach is to just declare the function signatures
> > in ks8851.h and provide non-static implementations in
> > ks8851_spi.c and ks8851_mll.c, so I'd strongly prefer that.
> > 
> > Even better, since this only concerns the register accessors
> > (which are inlined anyway by the compiler), it would be best
> > to have them in header files (e.g. ks8851_spi.h / ks8851_par.h)
> > which are included by the common ks8851.c based on the target
> > which is being compiled.  That involves a bit of kbuild magic
> > though to generate two different .o from the same .c file,
> > each with specific "-include ..." CFLAGS.
> 
> Before we go down the complex and ugly path, can you check whether this
> actually has performance impact ? I would expect that since this is an
> SPI-connected device, this here shouldn't have that much impact. But I
> might be wrong, I don't have the hardware.

I can test it, but the devices are in the office, I won't return there
before Thursday.  That said, I don't think it's a proper approach to
make the code more expensive even though it's perfectly possible to
avoid any performance impact, and shrug off concerns with the argument
that the impact should be measured first.


> Also note that having this dereference in place, it permits me to easily
> implement accessors for both LE and BE variant of the parallel bus device.

My understanding is that you're supposed to configure the chip to use
the native endianness of your architecture on ->probe() such that
conversions become unnecessary and the same accessor can be used for
LE and BE.  So why do you need two accessors?

Thanks,

Lukas