Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH net-next RFC v1 2/4] devlink: Add devlink traps under
 devlink_ports context
To:     Jiri Pirko <jiri@resnulli.us>, Ido Schimmel <idosch@idosch.org>
CC:     Aya Levin <ayal@mellanox.com>,
        "David S. Miller" <davem@davemloft.net>,
        Jakub Kicinski <kuba@kernel.org>,
        Jiri Pirko <jiri@mellanox.com>, <netdev@vger.kernel.org>,
        Moshe Shemesh <moshe@mellanox.com>,
        Eran Ben Elisha <eranbe@mellanox.com>,
        Ido Schimmel <idosch@mellanox.com>,
        <linux-kernel@vger.kernel.org>
References: <1599060734-26617-1-git-send-email-ayal@mellanox.com>
 <1599060734-26617-3-git-send-email-ayal@mellanox.com>
 <20200906154428.GA2431016@shredder> <20200908140409.GN2997@nanopsycho.orion>
From:   Aya Levin <ayal@nvidia.com>
Message-ID: <6aeba5b9-201f-2abc-05fb-0efdd6394b65@nvidia.com>
Date:   Thu, 10 Sep 2020 09:16:41 +0300
User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:68.0) Gecko/20100101
 Thunderbird/68.12.0
MIME-Version: 1.0
In-Reply-To: <20200908140409.GN2997@nanopsycho.orion>
Content-Type: text/plain; charset="utf-8"; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


On 9/8/2020 5:04 PM, Jiri Pirko wrote:
> Sun, Sep 06, 2020 at 05:44:28PM CEST, idosch@idosch.org wrote:
>> On Wed, Sep 02, 2020 at 06:32:12PM +0300, Aya Levin wrote:
> 
> [...]
> 
>>
>> I understand how this struct allows you to re-use a lot of code between
>> per-device and per-port traps, but it's mainly enabled by the fact that
>> you use the same netlink commands for both per-device and per-port
>> traps. Is this OK?
>>
>> I see this is already done for health reporters, but it's inconsistent
>> with the devlink-param API:
>>
>> DEVLINK_CMD_PARAM_GET
>> DEVLINK_CMD_PARAM_SET
>> DEVLINK_CMD_PARAM_NEW
>> DEVLINK_CMD_PARAM_DEL
>>
>> DEVLINK_CMD_PORT_PARAM_GET
>> DEVLINK_CMD_PORT_PARAM_SET
>> DEVLINK_CMD_PORT_PARAM_NEW
>> DEVLINK_CMD_PORT_PARAM_DEL
>>
>> And also with the general device/port commands:
>>
>> DEVLINK_CMD_GET
>> DEVLINK_CMD_SET
>> DEVLINK_CMD_NEW
>> DEVLINK_CMD_DEL
>>
>> DEVLINK_CMD_PORT_GET
>> DEVLINK_CMD_PORT_SET
>> DEVLINK_CMD_PORT_NEW
>> DEVLINK_CMD_PORT_DEL
>>
>> Wouldn't it be cleaner to add new commands?
>>
>> DEVLINK_CMD_PORT_TRAP_GET
>> DEVLINK_CMD_PORT_TRAP_SET
>> DEVLINK_CMD_PORT_TRAP_NEW
>> DEVLINK_CMD_PORT_TRAP_DEL
>>
>> I think the health API is the exception in this case and therefore might
>> not be the best thing to mimic. IIUC, existing per-port health reporters
>> were exposed as per-device and later moved to be exposed as per-port
>> [1]:
>>
>> "This patchset comes to fix a design issue as some health reporters
>> report on errors and run recovery on device level while the actual
>> functionality is on port level. As for the current implemented devlink
>> health reporters it is relevant only to Tx and Rx reporters of mlx5,
>> which has only one port, so no real effect on functionality, but this
>> should be fixed before more drivers will use devlink health reporters."
> 
> Yeah, this slipped trough my fingers unfortunatelly :/ But with
> introduction of per-port health reporters, we could introduce new
> commands, that would be no problem. Pity :/
> 
> 
>>
>> [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=ac4cd4781eacd1fd185c85522e869bd5d3254b96
>>
>> Since we still don't have per-port traps, we can design it better from
>> the start.
> 
> I agree. Let's have a separate commands for per-port.
Thanks for your input
I'm preparing V2
> 
> 
> [...]
>