Re: [PATCH rdma-next v1 11/11] RDMA/mlx5: Add support to get MR resource in RAW format

[Maor Gottlieb <maorg@mellanox.com>]

On 6/8/2020 2:46 PM, Jason Gunthorpe wrote:
> On Sun, Jun 07, 2020 at 11:47:11AM +0300, Maor Gottlieb wrote:
>> On 6/2/2020 4:23 PM, Leon Romanovsky wrote:
>>> On Tue, Jun 02, 2020 at 09:27:02AM -0300, Jason Gunthorpe wrote:
>>>> On Tue, Jun 02, 2020 at 09:21:18AM +0300, Leon Romanovsky wrote:
>>>>> On Mon, Jun 01, 2020 at 09:26:46AM -0300, Jason Gunthorpe wrote:
>>>>>> On Sun, May 31, 2020 at 12:54:14PM +0300, Leon Romanovsky wrote:
>>>>>>> On Fri, May 29, 2020 at 08:31:21PM -0300, Jason Gunthorpe wrote:
>>>>>>>> On Wed, May 27, 2020 at 04:54:08PM +0300, Leon Romanovsky wrote:
>>>>>>>>> From: Maor Gottlieb <maorg@mellanox.com>
>>>>>>>>>
>>>>>>>>> Add support to get MR (mkey) resource dump in RAW format.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Maor Gottlieb <maorg@mellanox.com>
>>>>>>>>> Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
>>>>>>>>>    drivers/infiniband/hw/mlx5/restrack.c | 3 ++-
>>>>>>>>>    1 file changed, 2 insertions(+), 1 deletion(-)
>>>>>>>>>
>>>>>>>>> diff --git a/drivers/infiniband/hw/mlx5/restrack.c b/drivers/infiniband/hw/mlx5/restrack.c
>>>>>>>>> index 9e1389b8dd9f..834886536127 100644
>>>>>>>>> +++ b/drivers/infiniband/hw/mlx5/restrack.c
>>>>>>>>> @@ -116,7 +116,8 @@ int mlx5_ib_fill_res_mr_entry(struct sk_buff *msg,
>>>>>>>>>    	struct nlattr *table_attr;
>>>>>>>>>
>>>>>>>>>    	if (raw)
>>>>>>>>> -		return -EOPNOTSUPP;
>>>>>>>>> +		return fill_res_raw(msg, mr->dev, MLX5_SGMT_TYPE_PRM_QUERY_MKEY,
>>>>>>>>> +				    mlx5_mkey_to_idx(mr->mmkey.key));
>>>>>>>> None of the raw functions actually share any code with the non raw
>>>>>>>> part, why are the in the same function? In fact all the implemenations
>>>>>>>> just call some other function for raw.
>>>>>>>>
>>>>>>>> To me this looks like they should should all be a new op
>>>>>>>> 'fill_raw_res_mr_entry' and drop the 'bool'
>>>>>>> I don't think that this is right approach, we already created ops per-objects
>>>>>>> o remove API multiplexing. Extra de-duplication will create too much ops
>>>>>>> without any real benefit.
>>>>>> If there is no code sharing then they should not be in the same
>>>>>> function at all. More ops is not really a problem.
>>>>> Logically they are the same, user asks to get object property, driver returns.
>>>> I'm starting to think it is also a mistake to have the same netlink op
>>>> and trigger it by an inbound attribute. Are there other examples of
>>>> that in netlink? Feels wrong
>>> I have no idea, don't see it in devlink.c
>> Jason, do you mean trigger the raw by inbound attribute? I don't see a
>> reason why not do that. Netlink attributes used for input and
>> output.
> What examples do you have where the input attribute completely changes
> the output format?
>
> Jason

I don't have. Anyway I am planning to send v2 with new netlink ops.