netdev - Re: [DISCUSS] sfp: sfp controller concept

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: <20200912091242.GI1551@shell.armlinux.org.uk>
Date:   Sat, 12 Sep 2020 10:12:42 +0100
From:   Russell King - ARM Linux admin <linux@...linux.org.uk>
To:     Vadym Kochan <vadym.kochan@...ision.eu>
Cc:     netdev@...r.kernel.org, Andrew Lunn <andrew@...n.ch>
Subject: Re: [DISCUSS] sfp: sfp controller concept

On Fri, Sep 11, 2020 at 09:19:14PM +0300, Vadym Kochan wrote:
> Hi,
> 
> I'd like to discuss a concept of introduction additional entity into SFP
> subsystem called SFP controller. But lets start with the issue.
> 
> Issue
> =====
> 
> There are boards with SFP ports whose GPIO pins are not connected directly to
> the SoC but to the I2C CPLD device which has ability to read/write statuses of
> each SFP via I2C read/write commands.
> 
> Of course there is already a solution - implement GPIO chip and convert GPIO
> numbers & states into internal representation (I2C registers). But it requires
> additional GPIO-related handling like:
> 
> 1) Describe GPIO pins and IN/OUT mapping in DTS
> 
> 2) Consider this mapping also in CPLD driver
> 
> 3) IRQ - for me this is not clear how to simulate
>    sending IRQ via GPIO chip.
> 
> I started to think that may be it would be good to introduce
> some generic sfp_controller which might be registered by such CPLD
> driver which may provide states of connected modules through the
> callback by mapping direct SFP states into it's CPLD registers and
> call some sfp_state_update() which will trigger the SFP state
> machine. So this driver will check/provide on direct SFP defined
> states without considering the GPIO things.

The driver already has the basis for splitting the control signals -
this is why there is sfp->get_state()/sfp->set_state(). However, until
there is hardware that requires something that isn't a GPIO, there was
no point taking it further.

> How it may look
> ===============
> 
> Device tree:
> 
> sfp0: sfp0 {
>         compatible = "sff,sfp";
>         i2c-bus = <&i2c0_sfp0>;
>         /* ref to controller device */
>         ctl = <&cpld>;
>         /* this index will be used by sfp controller */
>         idx = <0>;
> };
> 
> SFP controller interface:
> 
> There might be added generic sfp-ctl.c which implements the basic sfp controller infra:
> 
>     1) register/unregister sfp controller driver
> 
>     2) lookup sfp controller by fwnode on SFP node parsing/probing
> 
> The relation between modules might be:
> 
>     sfp.c <-> sfp-ctl.c <- driver <-> CPLD or some device
> 
> Flows:
> 1) CPLD driver prope:
>     driver -> sfp_controller_register()
> 
> 2) SFP instance probe:
>     sfp.c -> sfp-ctl.c:sfp_controller_add_socket()
>              creates assoctation between idx and sfp instance.
>                                       
> 3) SFP get state:
>     sfp.c -> sfp_ctl_get_state() -> sfp-ctl.c:sfp_controller_get_state() -> driver ops -> get_state
> 
> 4) SFP state updated:
>     driver -> sfp-ctl.c:sfp_controller_notify() -> sfp.c:sfp_state_update()
>               finds struct sfp* instance by idx

You are missing one of the most important things to consider - the SFP
and SFP controller become different drivers. How do they get associated,
and how does the probing between both work? What happens when one of
those drivers is unbound and the resources it provides are taken away?

> Currently I do not see how to properly define sfp_state_update(...) func
> which may be triggered by sfp controller to notify SFP state machine. May be additional
> interface is needed which may provide to controller the sfp* instance and it's idx:
> 
> int sfp_controller_add_socket(struct sfp_controller *ctl, struct sfp *sfp, int idx);
> 
> void sfp_controller_del_socket(struct sfp_controller *ctl, struct sfp *sfp);

I don't see how an index helps. What does the index define?

There's also the issue that the SFP cage driver needs to know which
hardware signals are implemented, so it knows whether to make use of the
soft signals that are available via the I2C bus on the module. I don't
see that having been addressed in your proposal.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!