Hi Alan,
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 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.
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. 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 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. 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.
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);
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);
The calling functions _have_ the msk_if_softc already. Just make msk_phy_readreg() take a msk_if_softc pointer (like the BSD driver does), and there would be no lookup required.
Thanks Alan.
You guys know this code way better than I do, but I tend to agree with Alan that
a) it makes sense to follow the FreeBSD code more closely b) the patch in its current form mixes too many types of changes c) adding linked lists to look up values that could be obtained in a more straightforward way seems unwise. _______________________________________________ Linaro-uefi mailing list Linaro-uefi@lists.linaro.org https://lists.linaro.org/mailman/listinfo/linaro-uefi