netdev - Re: [PATCH] net: allow shifted address in smsc911x

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] [day] [month] [year] [list]

Message-ID: <AANLkTinkqvXsyrWdSN9KP+6iZnchvnn8=UECwOcKz6tP@mail.gmail.com>
Date:	Tue, 29 Mar 2011 15:17:22 -0600
From:	Mathieu Poirier <mathieu.poirier@...aro.org>
To:	David Miller <davem@...emloft.net>
Cc:	steve.glendinning@...c.com, netdev@...r.kernel.org,
	lee.jones@...aro.org, patches@...aro.org, linus.walleij@...aro.org
Subject: Re: [PATCH] net: allow shifted address in smsc911x

My first email bounced on the list, hence sending again.

Please see my comments below.

Thanks,
Mathieu.

On 25 March 2011 15:31, David Miller <davem@...emloft.net> wrote:
> From: mathieu.poirier@...aro.org
> Date: Fri, 25 Mar 2011 15:27:17 -0600
>
>> From: Alessandro Rubini <rubini@...dd.com>
>>
>> At least one device I'm using needs address shifting to access
>> registers in the LAN9221 device (smsc911x driver).  This patch
>> adds a shift parameter in platform_data. The feature must be
>> enabled at configuration time, because shifting by a pdata
>> parameter makes access slower in those devices where no shift is
>> needed (I tested one, it was 20% slower).
>>
>> If the platform data requests shifted access but the feature is
>> unavailable, the probe method complains to the console. Board
>> config files should set the configuration option when needed.
>>
>> Signed-off-by: Alessandro Rubini <rubini@...dd.com>
>> Signed-off-by: Mathieu Poirier <mathieu.poirier@...aro.org>
>
> Implement this at run time, keying off of a boolean or similar
> in the platform data.
>
> Implement a set of ops, one to do a PIO read one to do a PIO write,
> and choose the set of ops appropriate for the device depending upon
> the setting found in the platform device.
>

I'm find with this approach but confused on the way to implement it.

I could add the shifted read/write functions in smsc911x.c and choose,
based on a boolean found in the 'smsc911x_platform_config' struct,
which will be used (the original or the shifted methods).  The
drawback with this approach is the new shifted read/write methods are
polluting smsc911x.c, something I'm not so fond of.  It might be
acceptable for one set of OPS but what if there is many more ?

A better approach would be to pass the read/write methods to the
smsc911x core using the 'smsc911x_platform_config' structure.  That
way smsc911x.c is kept clean but we'd have to move 'struct
smsc911x_data' to smsc911x.h to implement the methods from the board
files, something I'm not about do to without some sort of consensus.

'smsc911x_tx_writefifo' and 'smsc911x_rx_readfifo' are also of concern
to me.  I understand the usage of writesl/readsl but it introduces
more challenges.  If we don't want to replace their usage by a while
loop as it is the case when SMSC911X_SWAP_FIFO and SMSC911X_USE_16BIT
are set, we will need to introduce rx_readfifo and rx_writefifo in the
OPS.

Enlightenment is required on the above.

Best regards,
Mathieu.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html