Subject: Re: [PATCH net-next] rocker: check for BRIDGE_FLAGS_SELF in bridge setlink handler

From: roopa <[email-masked]>
Date: Thu, 05 Mar 2015 13:40:22 -0800

On 3/5/15, 12:43 PM, roopa wrote:
> On 3/5/15, 12:06 PM, Scott Feldman wrote:
>> On Thu, Mar 5, 2015 at 6:55 AM, roopa <[email-masked]> wrote:
>>> On 3/5/15, 12:02 AM, Jiri Pirko wrote:
>>>> Wed, Mar 04, 2015 at 09:51:20AM CET, [email-masked] wrote:
>>>>> On 3/3/15, 11:02 PM, Scott Feldman wrote:
>> [cut]
>>
>>>>>> Rocker setlink wasn't broken prior to the NETIF_F_HW_SWITCH_OFFLOAD
>>>>>> patches.
>>>>>>    Now it is,
>>>>> sure, I can submit a patch to remove the flag on rocker ports if 
>>>>> thats
>>>>> what
>>>>> you prefer.
>>>> I don't believe that Scott prefers that. The offload flag is there not
>>>> only for this case, but also for many future switch offloading cases.
>>> I very well understand that.
>>>> Rocker port have to have that port set by default. I do not really
>>>> understand why you suggest removing it...
>>>>
>>> I was just making sure scott is on board with the use of the flag.
>>> I added it on rocker ports for l2 and learning might be broken 
>>> because of
>>> that. Which i am trying to fix.
>>> also, scotts l3 patches dont seem to use the flag.
>> That's an oversight due to work starting on L3 patches way before
>> NETIF_F_HW_SWITCH_OFFLOAD was introduced ; I'll add checks for that
>> flag to the L3 patches.
> ok, i was not sure if you were planning to. I did post a comment on v2.
>>> So, if the patch in this thread is not the right way to fix it..,.i 
>>> was just
>>> trying to give scott an option to remove the flag to keep l2 working 
>>> and for
>>> him to bring back the flag when he is ready.
>> When I'm ready for what?  I ready for it to work like it did before
>> NETIF_F_HW_SWITCH_OFFLOAD and your setlink changes went in.  ;-)
> well, to be fair scott, my current patch in this thread is trying to 
> get to it, if you let me :)
>
>>
>> We want two sets of IFLA_PROTINFO attrs for a bridge port.   The first
>> set is the bridge side of the port, so these attrs like learning or
>> flooding control the how the bridge (software) manages the port.  The
>> second set is the device side of the port, so attrs here control how
>> the (offload) device manages the port.  The two sets are independent.
>> For example, you could have learning turned OFF on the bridge side and
>> learning turned ON on the device side.  The device side will
>> initialize its set.  The bridge will initialize its set.   The user
>> can see both sets with cmd
>>
>>      bridge -d link show.
>>
>> The user can set the bridge's attr with one of cmds:
>>
>>      bridge link set <attr> dev <port>
>>      bridge link set <attr> dev <port> master
>>
>> The user can set the device's attr with cmd:
>>
>>      bridge link set <attr> dev <port> self
>>
>> The driver setlink/getlink ops should only be called in the SELF
>> context because the driver is the SELF side of the port. Somehow we
>> got away from that.
> True, If all attributes that the bridge setlink sets need to go 
> separately to kernel and hardware and
>  we will never mirror them. That is not the case however. vlans is an 
> example and there will be more bridge port attributes in the future.
>
>>   We talked about IFLA_AF_SPEC being handled
>> differently for VLANs.  For IFLA_PROTINFO, what do we need to get back
>> to what I have above?
>
> If I understand you correctly, you are saying that ndo_bridge_setlink 
> when it comes to offloads
> should only be used for IFLA_PROTINFO ?
>
and...since you brought up IFLA_PROTINFO...i would like to also add 
that, .....my rocker patch in this current thread is nothing but rocker 
driver telling that "i accept IFLA_PROTINFO attributes only if the 
BRIDGE_FLAGS_SELF is set". I had the check around IFLA_PROTINFO 
initially...but since you only seem to be looking at IFLA_PROTINFO in 
the function, I put the check at the beginning of the function.

Thanks,
Roopa