Re: MAC address set requires decision

[Bruce Richardson <bruce.richardson@intel.com>]

On Thu, Feb 23, 2023 at 02:18:59PM +0000, Ferruh Yigit wrote:
> On 2/23/2023 4:32 AM, Hemant Agrawal wrote:
> > 
> > On 22-Feb-23 11:25 PM, Honnappa Nagarahalli wrote:
> >> -----Original Message-----
> >>> From: Richardson, Bruce <bruce.richardson@intel.com>
> >>> Sent: Thursday, February 16, 2023 9:05 AM
> >>> To: Ferruh Yigit <ferruh.yigit@amd.com>; techboard@dpdk.org
> >>> Cc: Huisong Li <lihuisong@huawei.com>; Chengwen Feng
> >>> <fengchengwen@huawei.com>
> >>> Subject: RE: MAC address set requires decision
> >>>
> >>> Alternative suggestions:
> >>>
> >>> 1. Don't allow "set" of mac address to value already in the list. The user must
> >>> delete the entry manually first before adding it. Similarly, "add" fails if no
> >>> default mac address is set. This ensures consistency by enforcing strict
> >>> separation between the default mac address and the extra mac addresses.
> >>> You can't have extra addresses without a default, and you can't have
> >>> duplicates.
> >>>
> >>> 2. Always enforce overlap between the two lists - once default mac address is
> >>> set (automatically adding it to the mac addresses list), you can only replace
> >>> the default mac address by using an already-added one to the list. In this
> >>> case, the default address is only really an index into the address list, and no
> >>> deletion ever occurs.
> >>>
> >>> All the solutions below seem rather mixed to me, I'd rather see either strict
> >>> overlap, or strict non-overlap. Both these cases make it that you need more
> >>> calls to do certain tasks, e.g. with #2 to just replace mac address, you need to
> >>> add, set, then delete, but we can always add new, clearly named APIs, to do
> >>> these compound ops. On the plus side, with #2 we could make things doubly
> >>> clear by changing the parameter type of "set" to be an index, rather than
> >>> explicit mac, to make it clear what is happening, that you are choosing a
> >>> default mac from a list of pre-configured options.
> >>>
> >>> Regards,
> >>> /Bruce
> > 
> > Both of the above option seems good.  The option #1 above is safe, where
> > you are making the mac address set as independent of mac filtering. Also
> > making sure that mac filter are not messed up. However, the application
> > needs to add error handling now to delete and set.
> > 
> > In the option #2,  I  assume, it will provide full backward
> > compatibility i.e. the ethernet library can take care of the logic and
> > application need not to implement anything extra ? If that is the case,
> > it seems to be best.
> > 
> 
> I think #2 is not fully backward compatible,
> 
> Previously set() replaces MAC in list[0], so multiple set() commands end
> up with single MAC address. So device will filter only one MAC.
> 
> With #2, after first set(), application will need to add() first, later
> set() and del() old MAC, if I understand correctly.
> 
> prev:
> set(MAC1)
> set(MAC2)
> set(MAC3)
> 
> becomes:
> set(MAC1)
> add(MAC2)
> set(MAC2)
> del(MAC1)
> add(MAC3)
> set(MAC3)
> del(MAC2)
> 
> 
> Hence I think this complicates application that wants to just update
> default MAC.
> 
> 
I agree. I tend to think that #1 is the simplest and most
backward-compatible of the options. The only case where we end up with
different behaviour is the problematic (and already ambiguous) case where
one attempts to set a default mac that is already specified in the
"alternative" mac list for the card. It keeps all the simple cases as-is.

/Bruce