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: <a071d1a4-c627-f2e2-d689-4663671d97d9@denx.de>
Date:   Tue, 24 Mar 2020 15:10:59 +0100
From:   Marek Vasut <marex@...x.de>
To:     Lukas Wunner <lukas@...ner.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 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.

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

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.