Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <8e0a0ceb-5816-60a6-6219-7306e75ce006@nvidia.com>
Date:   Sat, 30 Jul 2022 18:54:52 -0700
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.5.0
Subject: Re: [PATCH net-next 0/3] net: vlan: fix bridge binding behavior and
 add selftests
Content-Language: en-US
To:     Sevinj Aghayeva <sevinj.aghayeva@gmail.com>,
        Nikolay Aleksandrov <razor@blackwall.org>
Cc:     aroulin@nvidia.com, sbrivio@redhat.com,
        "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org, bridge@lists.linux-foundation.org
References: <cover.1659195179.git.sevinj.aghayeva@gmail.com>
 <f7ede054-f0b3-558a-091f-04b4f7139564@blackwall.org>
 <CAMWRUK5j4UAwjw4UGN=SVbbDbut0zWg5e03wupAXCPwT8K8zzQ@mail.gmail.com>
 <CAMWRUK5TZ5iZWZJO7Bbn-b43ZbT7mRzUDr4LdseLCne8qvG6pw@mail.gmail.com>
From:   Roopa Prabhu <roopa@nvidia.com>
In-Reply-To: <CAMWRUK5TZ5iZWZJO7Bbn-b43ZbT7mRzUDr4LdseLCne8qvG6pw@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?K0tYTGIzL1c3VG9FVzF5dlNlVldzcDNGdENaYlNJKzA5ZlZmaTNRMG5xRnNO?=
 =?utf-8?B?ak5KREN0MC9MTjNWZmpSNkw1NzJGZlljS1NIK2xQeTFWbC9MTTFrT1daNUVp?=
 =?utf-8?B?VXhWcGlML2g5VWVkcE9IVnZoZ3lBd3RNQUNGazB6NVdSVkJrWUczZVpUSFpH?=
 =?utf-8?B?WDVDRVU4MEwxM3ptYW5TOWcvK25MRDhQVTJxaGgwWlczRE85WitnNkNNQ213?=
 =?utf-8?B?VWR3RSttWGFDVWFWUmRCUDlLaktBeVErdDFsYWE0N3VnUmZCV3kvaTNVcGQv?=
 =?utf-8?B?SGRPRGNyMnF6NDd5alJsbEk3K3p4Z2d0SWJWTERPOGd6RGwwSlU5eExCVVFM?=
 =?utf-8?B?WmQwcHNSV3VPWXc1UzdvRm5XLzltN3JQQnpBcUdsRVBuM0orMDlGY0l6YXkr?=
 =?utf-8?B?UGo3amhDTkRVNnZDb2lHc0ZRVjJOcmpwcjZaQkVZeERyYUJSRjFGSGxXSmJ5?=
 =?utf-8?B?dm12TC9ObW5wYlo5SmtUYmYzSWhFSGNZRDVvWWpDcjBMU1VBV3ExR2xVdXJh?=
 =?utf-8?B?OXVkYzQwOXUxb0Y5UW5oWE05MThMSjFBWFU1ZGVFTFM2cTlYSW1zbzg3Y1da?=
 =?utf-8?B?aUI5M3d1NDVlenBMcXpINmRmcUtVY2lIVVdtTHVpeTF4L3JZaEJkdWpIZ2Vh?=
 =?utf-8?B?RDBxc29ZaDdVaVlwV3hmWkJiQjhEMGJ4Umx0SGo0cmlUVXpoY1U2RXd4WE9I?=
 =?utf-8?B?T0o3cVQ1M0hDWkdja21nWmhRckFHeWtrc0xCTEZHalNoeDhYTUk1Zk9YVGF3?=
 =?utf-8?B?K0c3WXUwMUFGRU1ydnl1b1hLV3A2TlZEMHp4bEdJeEFrcHBZNmpVbVBkMVNZ?=
 =?utf-8?B?ZnJNN3g1aDFPZElsTTQ2anh6cE9SeVpLS1hPWTB5M1Jtd3lDZTV4RTZPOW4r?=
 =?utf-8?B?NHNBWk11b3ovY2ZOaWIyZmNmc2d4QlluRXVIcG11Z1YzSXVtVzN1OENOdDNv?=
 =?utf-8?B?dFJ3VzRQSEhvTTFaVXhQY1FiN0FLZ1k4cElVYkJoZGtWYktJZ0R5aVBHc2E3?=
 =?utf-8?B?cDRkVTR3cWVib2svMUFVd3hlb0tpQVFnRjE1OVVZZGljWU5lTEg4bEJoWWtV?=
 =?utf-8?B?QjNwaXNZL1FDN1o4UHVpU3JrOTIrZWpKRkhaVHpkZDBQOGZiUnVjLzQvdTJK?=
 =?utf-8?B?SFZkRWtIK2hFS0dFeGdZL3A5MWVvSlNZajhLVVhLWXVoTDl2UVNRWEl5VEUw?=
 =?utf-8?B?cUJvckQ1cVdPRy9tOFpiVVVzc2ZaemkrZ3g2YUJtZ2U4ZEhVR3E3d2tKamRk?=
 =?utf-8?B?VU5DcW9OY0NMUlliSm1uK1hkZyszTmxITFNtUXN6aUFFUGNoNTVvTHRIWkIr?=
 =?utf-8?B?RC9lUWNUbmwzRkM5R0JLU25EdmdBYzlLVWxlaUNONHR5Y0wvbDl1YXFqVjQz?=
 =?utf-8?B?SkovZUJHRjE5SG1PT3l5MFYrNjlnK0MvV3gvdVhlWEpJT01tcXdmWHBMQ3Y3?=
 =?utf-8?B?Zk5sSFBuZkpKUnFibkYvaThmY08zMENRYTI2akpTTml5WDhFbFhVbzRHMTFN?=
 =?utf-8?B?VFFVZUdBOGQ2MG55MHdmZGlZSm5PWXhSb0Rlak1IYVhFNCtKbmxkK1RCV2c4?=
 =?utf-8?B?QnFiRUtkQTJHeHlsRDQ2ak1wTy9td21aNUdEQ2FCQXRRb3BQU2VOUlJZTXBZ?=
 =?utf-8?B?TVNlNXo0YWR0OVVCTFUrQWRTTlgxQW9xRFA3K1FLQm1xZkdPZ1JLUGwzdnRY?=
 =?utf-8?B?RjZUY0k0UnBsVG1vMUc3Zm9sMk1sQnRzOGdkdk5kZVBXZ0QwMU9EUjZEdmNq?=
 =?utf-8?B?blowVjdPa0JoaWRPd05TbDNSNFA3emNwV3drM0prcjVma3NUUTNDbGMrQ1J4?=
 =?utf-8?B?cC9za2x1bWNmVDR3dHBjdFFXSll2UkFOcjZkVSsxdGJzZDhIaUJDSWIyY1JT?=
 =?utf-8?B?QzJJZFlkcWZOc0lOV1ZlMzFJRkNCTkZRM0VEd0hPenAyR0M2VTh2VG9xZkVW?=
 =?utf-8?B?VGladXhzeWlUcEF4RE81WnVIOGlPbGdicEFXTXlCOXAwZzVqYkhxWXphSDEv?=
 =?utf-8?B?YWhvNFc3UStlRDc1cTlibTJwOTBTZ3c1R0RRaXRFVVBiNW1oQjZkdTNBbVRy?=
 =?utf-8?B?RWQ1RUh0NFlvNHZnKysrUm55c3hiblF0TnlGS25YZkxvS2RVOHRBeThxU0ZX?=
 =?utf-8?B?OWxEMDVZaWpWR21mS3Bqa2xPNkN2QngvTmx1MTVUZkIvdzhSZ2RlcXFrcnBW?=
 =?utf-8?Q?+rJJaa0b0FfPiEEarJ4jL4qLgt/NmOJo24euaPiSTj1B?=
X-OriginatorOrg: Nvidia.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 265bfab0-9b6b-4ddf-2365-08da7297a998
X-MS-Exchange-CrossTenant-AuthSource: SJ0PR12MB5504.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 31 Jul 2022 01:54:53.8700
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 43083d15-7273-40c1-b7db-39efd9ccc17a
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: Bu3JVZsj2zU/xuNxPMaNq/DRpiSUjDo8a/sqecUEP7ZlpTSPg89CaiMx5wlKpzb2ED1hkAt5Y1t7Jb2cq+XAKg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN6PR12MB1716
X-Spam-Status: No, score=-1.7 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FORGED_SPF_HELO,
        NICE_REPLY_A,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,
        SPF_NONE autolearn=no autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


On 7/30/22 09:48, Sevinj Aghayeva wrote:
> (Resending this because the first email was rejected due to being in HTML.)
>
>
> On Sat, Jul 30, 2022 at 12:46 PM Sevinj Aghayeva
> <sevinj.aghayeva@gmail.com> wrote:
>>
>>
>> On Sat, Jul 30, 2022 at 12:22 PM Nikolay Aleksandrov <razor@blackwall.org> wrote:
>>> On 7/30/22 19:03, Sevinj Aghayeva wrote:
>>>> When bridge binding is enabled for a vlan interface, it is expected
>>>> that the link state of the vlan interface will track the subset of the
>>>> ports that are also members of the corresponding vlan, rather than
>>>> that of all ports.
>>>>
>>>> Currently, this feature works as expected when a vlan interface is
>>>> created with bridge binding enabled:
>>>>
>>>>     ip link add link br name vlan10 type vlan id 10 protocol 802.1q \
>>>>           bridge_binding on
>>>>
>>>> However, the feature does not work when a vlan interface is created
>>>> with bridge binding disabled, and then enabled later:
>>>>
>>>>     ip link add link br name vlan10 type vlan id 10 protocol 802.1q \
>>>>           bridge_binding off
>>>>     ip link set vlan10 type vlan bridge_binding on
>>>>
>>>> After these two commands, the link state of the vlan interface
>>>> continues to track that of all ports, which is inconsistent and
>>>> confusing to users. This series fixes this bug and introduces two
>>>> tests for the valid behavior.
>>>>
>>>> Sevinj Aghayeva (3):
>>>>     net: bridge: export br_vlan_upper_change
>>>>     net: 8021q: fix bridge binding behavior for vlan interfaces
>>>>     selftests: net: tests for bridge binding behavior
>>>>
>>>>    include/linux/if_bridge.h                     |   9 ++
>>>>    net/8021q/vlan.h                              |   2 +-
>>>>    net/8021q/vlan_dev.c                          |  21 ++-
>>>>    net/bridge/br_vlan.c                          |   7 +-
>>>>    tools/testing/selftests/net/Makefile          |   1 +
>>>>    .../selftests/net/bridge_vlan_binding_test.sh | 143 ++++++++++++++++++
>>>>    6 files changed, 176 insertions(+), 7 deletions(-)
>>>>    create mode 100755 tools/testing/selftests/net/bridge_vlan_binding_test.sh
>>>>
>>> Hmm.. I don't like this and don't think this bridge function should be
>>> exported at all.
>>>
>>> Calling bridge state changing functions from 8021q module is not the
>>> proper way to solve this. The problem is that the bridge doesn't know
>>> that the state has changed, so you can process NETDEV_CHANGE events and
>>> check for the bridge vlan which got its state changed and react based on
>>> it. I haven't checked in detail, but I think it should be doable. So all
>>> the logic is kept inside the bridge.
>>
>> Hi Nik,
>>
>> Can please elaborate on where I should process NETDEV_CHANGE events? I'm doing this as part of outreachy project and this is my first kernel task, so I don't know the bridging code that well.
>>
>> Thanks!

good point Nikolay.

Sevinj, see br_vlan_bridge_event and __vlan_device_event  for how both 
drivers react to netdev change events.

I have not looked at it in detail yet, but lets explore and discuss if 
we can make use of events to achieve same results.