netdev - Re: [PATCH V6 09/20] net: ks8851: Use 16-bit read of RXFC register

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:   Sun, 17 May 2020 22:02:09 +0200
From:   Marek Vasut <marex@...x.de>
To:     Andrew Lunn <andrew@...n.ch>
Cc:     netdev@...r.kernel.org, "David S . Miller" <davem@...emloft.net>,
        Lukas Wunner <lukas@...ner.de>, Petr Stetiar <ynezz@...e.cz>,
        YueHaibing <yuehaibing@...wei.com>
Subject: Re: [PATCH V6 09/20] net: ks8851: Use 16-bit read of RXFC register

On 5/17/20 9:44 PM, Andrew Lunn wrote:
> On Sun, May 17, 2020 at 02:33:43AM +0200, Marek Vasut wrote:
>> The RXFC register is the only one being read using 8-bit accessors.
>> To make it easier to support the 16-bit accesses used by the parallel
>> bus variant of KS8851, use 16-bit accessor to read RXFC register as
>> well as neighboring RXFCTR register.
>>
>> Remove ks8851_rdreg8() as it is not used anywhere anymore.
>>
>> There should be no functional change.
>>
>> Signed-off-by: Marek Vasut <marex@...x.de>
>> Cc: David S. Miller <davem@...emloft.net>
>> Cc: Lukas Wunner <lukas@...ner.de>
>> Cc: Petr Stetiar <ynezz@...e.cz>
>> Cc: YueHaibing <yuehaibing@...wei.com>
>> ---
>> V2: No change
>> V3: No change
>> V4: Drop the NOTE from the comment, the performance is OK
>> V5: No change
>> V6: No change
>> ---
>>  drivers/net/ethernet/micrel/ks8851.c | 17 +----------------
>>  1 file changed, 1 insertion(+), 16 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/micrel/ks8851.c b/drivers/net/ethernet/micrel/ks8851.c
>> index 1b81340e811f..e2e75041e931 100644
>> --- a/drivers/net/ethernet/micrel/ks8851.c
>> +++ b/drivers/net/ethernet/micrel/ks8851.c
>> @@ -236,21 +236,6 @@ static void ks8851_rdreg(struct ks8851_net *ks, unsigned op,
>>  		memcpy(rxb, trx + 2, rxl);
>>  }
>>  
>> -/**
>> - * ks8851_rdreg8 - read 8 bit register from device
>> - * @ks: The chip information
>> - * @reg: The register address
>> - *
>> - * Read a 8bit register from the chip, returning the result
>> -*/
>> -static unsigned ks8851_rdreg8(struct ks8851_net *ks, unsigned reg)
>> -{
>> -	u8 rxb[1];
>> -
>> -	ks8851_rdreg(ks, MK_OP(1 << (reg & 3), reg), rxb, 1);
>> -	return rxb[0];
>> -}
>> -
>>  /**
>>   * ks8851_rdreg16 - read 16 bit register from device
>>   * @ks: The chip information
>> @@ -470,7 +455,7 @@ static void ks8851_rx_pkts(struct ks8851_net *ks)
>>  	unsigned rxstat;
>>  	u8 *rxpkt;
>>  
>> -	rxfc = ks8851_rdreg8(ks, KS_RXFC);
>> +	rxfc = (ks8851_rdreg16(ks, KS_RXFCTR) >> 8) & 0xff;
>>  
>>  	netif_dbg(ks, rx_status, ks->netdev,
>>  		  "%s: %d packets\n", __func__, rxfc);
> 
> Hi Marek
> 
> This is the patch which i think it causing most problems. So why not
> add accessors ks8851_rd_rxfc_spi() and ks8851_rd_rxfc_par(), each
> doing which is optimal for each?

Because it makes no difference when I do iperf tests on current
linux-next. I think I mentioned this before a few times, but the real
performance improvement here would be gained if this whole driver was
converted to regmap and then the regmap core could merge the SPI
transfers as needed. But that is something for another series.