On 10/19/2016 03:44 PM, Daniil Egranov wrote:
On 10/19/2016 11:37 AM, Ard Biesheuvel wrote:
On 19 October 2016 at 05:32, Alan Ott alan@softiron.co.uk wrote:
On 10/17/2016 05:45 PM, Daniil Egranov wrote:
The patch adds support for multi-controller configuration.
Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Daniil Egranov daniil.egranov@arm.com
I tested this, and it does work on my hardware here.
Does it fail to detect the hardware or there is no communication? Do you have a dual MAC controller? I tested it on Juno with three Marvell cards and have not seen any issues.
I said it works. :)
I guess my biggest criticism of this patch is, why change all the function prototypes to take mSoftc, mController, and mPciIo parameters? In the FreeBSD[0] driver, the msk_softc is simply held by the msk_if_softc (which is the main object for each network controller (interface)). The PCI I/O could be held there too. Each function that needs a msk_softc can simply get it from the msk_if_softc when it needs it.
The FreeBSD driver, for the most part, passes around msk_if_softc pointers, which makes sense, since from those pointers you can get to everything else.
The same thing happens in the patch. The pointer to msk_softc passed around the functions. The MSK_LINKED_DRV_BUF structure is equal to FreeBSD msk_if_softc. The UEFI type variables kept outside of the original msk driver structures and passed as extra parameters to the functions.
What? No. the msk_if_softc in the UEFI driver is the same as the msk_if_softc in the FreeBSD driver. It's the same structure, and named the same thing. I'm saying to put a pointer to the msk_softc in the msk_if_softc, rather than passing the msk_softc (often named mSoftc) into every function that needs it.
I'm saying: 1. put a pointer to msk_softc into the msk_if_softc, like the FreeBSD driver has. 2. Pass msk_if_softc into functions like msk_phy_readreg(). See: https://github.com/freebsd/freebsd/blob/master/sys/dev/msk/if_msk.c#L409 3. In functions like msk_phy_readreg(): 3a. just use the pointer to the msk_softc from the msk_if_softc when you need it, just like the BSD driver. 3b. Don't look up the msk_softc using msk_get_ctrl_data_msks(), because it walks a list.
The first approach was to save a pointer to the msk_softc for each controller and hold it as part of the driver's binding structure.
I don't know what you mean by "driver's binding structure." Which one are you talking about?
However, the complication comes from the e1000phy.c. First, it has it's own PHY structure which has to be saved related to the controller. Second, it has dependency on the if_msk.c. It's calling functions from the if_msk.c which require access to the msk_softc structure so e1000phy has to tell the if_msk.c functions which msk_softc structure to use without having visibility to this structure. The mController is a common device descriptor between e1000phy and if_msk which binds them to the same controller (= dev in FreeBSD).
The phy functions have an mii_softc, so add the appropriate things to that structure (such as the msk_if_softc) instead of passing them into every single function. You can connect them in the _attach() functions
In the BSD driver, there's infrastructure to match the PHY to the MAC. In our case, they could easily be matched up by the calls to PHY_READ(), PHY_WRITE(). if those macros were to take an mii_softc and if mii_softc were modified to hold a pointer to an msk_if_softc.
The msk_softc and e1000phy_softc lookup calls minimized to the UEFI driver interface functions and if_msk.c functions used by e1000phy.
The approach is similar to the FreeBSD one but using linked list to store the data, i do not see a big difference. If you will check the linked list node lookup, it's about the same as the device_get_softc() but with extra security checking.
No, device_get_softc() just returns a data pointer. It doesn't have to walk a list. It's O(1) instead of O(n).
Also, the pointers to driver data structures stored privately inside of the linked list and not exposed to the outside world through SNP. The performance impact should be insignificant.
It's the complexity of the code that's the issue. It's complex when it doesn't need to be.
Is there a reason that would not work here? It seems like a much cleaner interface an fewer variables to keep track of and it wouldn't have to change function interfaces to have redundant parameters.
For example, have a look at FreeBSD's msk_phy_readreg() at [1]. It takes a msk_if_softc pointer and gets the msk_softc from it when it needs it. Your version of msk_phy_readreg() takes an mController parameter and then does a lookup of the msk_softc from a list[2] (every time you want to read a register!).
The same thing happens in FreeBSD driver, it returns the msk structure based on dev: struct msk_if_softc *sc_if; sc_if = device_get_softc(dev);
device_get_softc() just gets a pointer to a device's private data pointer: https://www.freebsd.org/cgi/man.cgi?query=device_get_softc&sektion=9
It's returning a pointer, not walking a list.
The UEFI driver does the same thing but adds extra security checking on the lookup: struct msk_softc *mSoftc; Status = msk_get_ctrl_data_msks (mController, &mSoftc);
These are not the same. One just returns a pointer that it has access to (FreeBSD), the other walks a list (UEFI).
Alan.