On 10/20/2016 10:14 PM, Alan Ott wrote:
On 10/20/2016 07:23 PM, Daniil Egranov wrote:
On 10/20/2016 09:13 AM, Alan Ott wrote:
On 10/19/2016 10:03 PM, Daniil Egranov wrote:
On 10/19/2016 04:12 PM, Alan Ott wrote:
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 misread it, sorry.
>> >> 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 am not talking about similarity between the UEFI msk_if_softc and FreeBSD msk_if_softc. I am talking about the MSK_LINKED_DRV_BUF structure containing a pointer to the msk_softc similar to the current FreeBSD msk_if_softc structure and used for the same purpose.
I'm saying:
- 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
- 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.
I understand what you say. You want to pass it as part of the structure vs pass it as a function parameter. I doubt that the code complexity will be different.
What's different is, the FreeBSD driver passes in what's needed instead of passing in a token that is used to walk a list to find the thing that that is needed. One is a whole lot more clear an concise.
It's more interesting which way will be better performance wise.
One executes in constant time, the other walks a list. One is constant time, the other is O(n).[1]
3b. Don't look up the msk_softc using msk_get_ctrl_data_msks(), because it walks a list.
The msk_get_ctrl_data_msks() is not called all the time. It use to get a msk_softc pointer only when the driver interface functions is called and with e1000phy/if_msk interface calls. In all other cases, a pointier to the msk_softc will be already available.
Your argument here seems to be that it's ok that the code is more complex and slow than it needs to be because it doesn't get called very often. The problem isn't the number of microseconds wasted; the problem is that it's more code and code complexity where there doesn't need to be code complexity.
You have multiple variables being thrown around: msk_softc (mSoftc) msk_if_softc (often sc_if) EFI_HANDLE (mController) EFI_PCI_PROTOCOL (mPciIo)
Put them in structures that logically make sense, rather than passing them all around separately. That's all I'm saying.
I don't know what kind of issue you have with the linked list. Please explain. The UEFI linked list is very lite and as i explained before, it adds the data integrity checking with a little overhead. The driver is using the linked list in several places already.
I don't have a problem with linked lists or the UEFI linked list implementation, but like any data structure, why should you have a linked list when you don't have to have a linked list? Why have a list and look-ups and lots of variables passed around independently of each other when you can do things like: sc_if->msk_softc instead?
What about multiple controllers. Where do you get a pointer to the sc_if for the particular controller?
In the BSD, there's an sc_if (msk_if_softc) for each logical interface (eth0:0 eth0:1, etc), and an sc (msk_softc) for each physical interface (eth0).
In our case, it's 1:1, since we don't have logical interfaces, so it's a 1:1 mapping. In the BSD they're linked together. sc_if has a pointer to the sc. We could (and should) easily have the same. It makes some of our problems easier.
We still have a case for a dual MAC controller (which i believe is Marvell Yukon XL). It's defined as array of pointers in the msk_softc so we potentially have 1:2 mapping with the common msk_softc structure for a controller. The msk_softc (or msk_if_softc) has to be stored for each controller and passed to the driver like FreeBSD does with device_t structure. It can be done similar way but I'll talk about it below. I do not have Marvell controller with two ports so only first MAC will be used for now but once I'll find such a controller, it will be enabled.
Prior to your patch, in the UEFI, we had an sc_if which was dynamically allocated in mskc_attach() and an sc which was static. That didn't make any sense.
Look at how they are assembled in the BSD in msk_attach(): https://github.com/freebsd/freebsd/blob/master/sys/dev/msk/if_msk.c#L1589
I went back to old versions of driver and both msk_softc and msk_if_softc were dynamically allocated with AllocatePool() in all versions of this driver. The layout of this driver has been done long time ago and based on pretty old version of FreeBSD driver. I believe it's been done specifically for Juno platform without intention to be used with other Marvell controllers. The driver can be reworked to make it more similar to the current FreeBSD one but it's another task.
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?
YUKON_DRIVER 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 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
Currently, there are only two functions with the mii_softc as a parameter. The other functions needing the e1000phy_softc have to be modified to have the mii_softc as a parameter. There is no difference between passing either e1000phy_softc, mii_softc, or mii_if_softc in this case.
Yes, that makes sense.
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.
No, I do not want to mix e1000phy and if_msk structures. The patch adds the controller id to PHY_READ() and PHY_WRITE() which is used to get a data pointer to the associated msk_softc structure. It's doing the same thing as you explained above without mixing the structures.
If you want to keep e1000phy and if_msk separate, then when you attach the phy, pass in what you need (mController, PciIo, etc), and store them somewhere in the phy's data structures. When possible, you should be passing one object around (preferably the one which describes the physical object you're working on, like the sc_if, or the mPhySoftC).
The mController object describes which mPhySoftC object should be used and then mPhySoftC object passed around. The mController passed from the if_msc for each controller instance. I do not see any difference in what the current patch code is doing and what you are saying.
There are only three places where phy functions are called from if_msk(). If you don't want to make e1000phy dependent on if_msk, then make those three entry points the places where a phy structure (mii_softc?) is looked up, and then pass that phy structure around when inside the e1000phy code. It's much more simple that way.
Our case is of course complicated by the fact that the BSD driver has infrastructure which connects the phy and the mac drivers so that each can exist without knowing about the other, and UEFI does not.
It's already done by passing the mController object from the e1000phy to if_msk. The if_msk functions lookup for the msk_softc using this object. It's pretty simple.
This is addressed in a comment below.
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.
I disagree. What you suggest has the same complexity. Also, what you suggest requires e1000phy_softc (for each port) and msk_softc pointers for each controller to be stored as part of the corresponding YUKON_DRIVER driver structure. As I explained above, I do not want to do it.
How does following a pointer to an object have the same code complexity as calling a function which walks a list to get a pointer to that object?
I think we are talking about two different things. Here is an example:
Here is the patched code:
static VOID clear_pci_errors ( EFI_PCI_IO_PROTOCOL *mPciIo, struct msk_softc *mSoftc ) { EFI_STATUS Status; UINT16 val;
....
}
Here is what you suggest:
static VOID clear_pci_errors ( struct msk_if_softc *sc_if ) { EFI_STATUS Status; UINT16 val; EFI_PCI_IO_PROTOCOL *mPciIo; struct msk_softc *mSoftc;
mPciIo = sc_if->mPciIo; mSoftc = sc_if->mSoftc; ....
}
Yes, that's exactly what I'm suggesting. The difference is that the interface is cleaner. You don't pass in an sc_if _and_ a PciIo, you just pass in an sc_if which _contains_ a PciIo.
When you say we are talking about different things, yes, there are multiple issues that I'm discussing. One is like you showed above, where it can simply be a matter of having the objects laid out a little differently to make the interfaces cleaner (and the patch smaller). Then there are other, different places where lookups are done instead. It's a large patch and you're doing a lot of different things in it, which is why we have some miscommunication.
I do not mind putting it inside of the structure. It's a matter of preference of defining an interface so which way is cleaner is subjective. In this case it make sense to follow the FreeBSD style, I'll rework it. However, I am not sure about the patch size with this changes. The parameters for several functions have to be changed, extracting pointers from the structure will add more code to each function. Also, mPort variable cannot be put inside of the structure as it points to the msk_if_softc defined in the msk_softc. The mPort comes as a driver external parameter and points to controller's msk_if_softc structure so it has to be kept as a function parameter.
There is no walk a list in most of the functions. Why do you think the complexity is different?
The complexity is different in the places where the list is walked when it doesn't have to be. I have showed you an example of this above. Not all the cases are the same in your code. Like I said there are multiple issues being discussed.
Your suggestion will not work in places where msk_if_softc is not allocated or accessible:
// Reset the adapter mskc_reset (mPciIo, mSoftc);
mskc_setup_rambuffer (mSoftc);
Status = gBS->AllocatePool (EfiBootServicesData, sizeof (struct *msk_if_softc*), (VOID**) &*ScIf*);
I agree, but have a look at how the BSD driver works here.
mSoftC and mPciIo should go with the msk_softc (sc) rather than the msk_if_softc, and by that point you _do_ have an msk_softc.
In our case since interface (eth0:0, eth0:1, etc) and hardware (eth0) are 1:1, mskc_attach() and msk_attach() have kind of become mixed up together :/ .
Now we are talking about a multi controller case and not a single instance case. The walk a list happens only in 5 interface functions used by SNP protocol functions and e1000phy/if_msk interface functions which do not have common objects. Once you have multiple controllers, the driver has to build the binding: EFI_HANDLE Controller(n) <-> msk_softc (n), e1000phy_softc(n) (x2 with two MACs) and store it. The purpose of the linked list is to keep these bindings. It has to be done one way or another, will it be a list or something else. Once the SNP function called, it passes the UEFI controller handle to the driver. The driver uses the corresponding msk_softc and e1000phy_softc stored in the linked list, it is simple like that. To be honest, i do not see any complexity in that.
There are places where it's done where it's done where it doesn't have to be, like the example I gave. There are others. That's what I'm saying.
I think this can be done without a mapping.
What if the sc_if was created in mskc_attach() (where it is already) and returned and stored in the YUKON_DRIVER. That way all the calls to SnpTransmit(), SnpReceive(), etc., already have the sc_if, and it can be passed directly into the msk_* functions? I think that would fix almost all the issues.
I agree. That is a best way for keeping driver's data associated with a controller. Storing the driver's data in YUKON_DRIVER I mention when i was talking about driver's binding structure earlier but there are couple concerns about it : 1. The driver's internal data can be access using the SNP pointer with the YUKON_DEV_FROM_THIS_SNP similar macro. Registering this data with an external protocol brings some security questions. That's a reason why i do not like to include it into the YUKON_DRIVER structure and want keeping it local to the driver in linked list or other form. 2. The e1000phy to if_msk interface without a shared object between them . That brings a complication to multiple controllers case. Fixing it may require significant changes in the driver structure.
To solve these two problems, the local linked list with the controller object was a simplest way with the current structure of the driver.
I will send another patch with the driver structures changes as you suggested, with name correction suggested by Ard, and with keeping linked list storage for the SNP interface function for now.
>> 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).
The result is the same. The msk_get_ctrl_data_msks has extra steps and i already explained why.
Yes extra steps and extra code that is not needed if you lay out the structures the right way. Look back at my example of msk_phy_readreg():
The lookup happens only in several cases when the structures are not directly accessible even if structures have a "right way" lay out. You still compare it to the new FreeBSD driver. This is the original UEFI driver msk_phy_readreg() function which is called from the e1000phy. The e1000phy does not know anything about msk_if_softc or msk_softc but as far as the mSoftc was global, it worked but now it has to lookup for mSoftc as you have in the patched version:
msk_phy_readreg ( INTN port, INTN reg ) { INTN i; INTN val;
GMAC_WRITE_2 (mSoftc, port, GM_SMI_CTRL, GM_SMI_CT_PHY_AD(PHY_ADDR_MARV) | GM_SMI_CT_REG_AD(reg) | GM_SMI_CT_OP_RD);
... }
The BSD driver's msk_phy_readreg() takes an sc_if. Could the phy code hold an opaque pointer to an sc_if, (or a void pointer) that the if_msk code passes to it (and then the phy code passes back) as a way of keeping the if_msk and the phy code loosely coupled?
Yes, that's an option. I am trying to avoid opaque pointers where its possible allowing a complier correctly track any type specific issues. I'll review this part of the code. It may remove msk_softc structure lookup here and do it only in the SNP interface functions.
In FreeBSD, it looks like this: static int msk_phy_readreg(struct msk_if_softc *sc_if, int phy, int reg) { struct msk_softc *sc; int i, val;
sc = sc_if->msk_softc; ... }In your patch, it looks like this: INTN msk_phy_readreg ( INTN port, INTN reg, EFI_HANDLE mController ) { INTN i; INTN val; EFI_STATUS Status; struct msk_softc *mSoftc;
Status = msk_get_ctrl_data_msks (mController, &mSoftc); ... }It has nothing to do with device_get_softc(). It has to do with the structures being laid out properly so that you have the data you need, rather than having to walk a list to look it up. The BSD version says sc_if->msk_softc, and your version says msk_get_ctrl_data_msks (mController, &mSoftc) which walks a list.
Alan.
[1] It is a small n, but even so, why have O(n) of any size when it can be O(1)?
Linaro-uefi mailing list Linaro-uefi@lists.linaro.org https://lists.linaro.org/mailman/listinfo/linaro-uefi
Thanks, Daniil
Linaro-uefi mailing list Linaro-uefi@lists.linaro.org https://lists.linaro.org/mailman/listinfo/linaro-uefi