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:
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.

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.

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

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.

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.

 

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?

 

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