Re: [PATCH 1/6] vdpa: fix improper error message when adding vdpa dev

[Si-Wei Liu]

On 1/31/2023 3:42 AM, Eli Cohen wrote:
> On 30/01/2023 22:30, Si-Wei Liu wrote:
> > In below example, before the fix, mtu attribute is supported
> > by the parent mgmtdev, but the error message showing "All
> > provided are not supported" is just misleading.
> >
> > $ vdpa mgmtdev show
> > vdpasim_net:
> >    supported_classes net
> >    max_supported_vqs 3
> >    dev_features MTU MAC CTRL_VQ CTRL_MAC_ADDR ANY_LAYOUT VERSION_1 
> > ACCESS_PLATFORM
> >
> > $ vdpa dev add mgmtdev vdpasim_net name vdpasim0 mtu 5000 max_vqp 2
> > Error: vdpa: All provided attributes are not supported.
> > kernel answers: Operation not supported
> >
> > After fix, the relevant error message will be like:
> >
> > $ vdpa dev add mgmtdev vdpasim_net name vdpasim0 mtu 5000 max_vqp 2
> > Error: vdpa: Some provided attributes are not supported.
> > kernel answers: Operation not supported
> >
> > $ vdpa dev add mgmtdev vdpasim_net name vdpasim0 max_vqp 2
> > Error: vdpa: All provided attributes are not supported.
> > kernel answers: Operation not supported
> >
> > Signed-off-by: Si-Wei Liu <si-wei.liu@xxxxxxxxxx>
> > Acked-by: Jason Wang <jasowang@xxxxxxxxxx>
> > ---
> >   drivers/vdpa/vdpa.c | 9 ++++++++-
> >   1 file changed, 8 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
> > index 8ef7aa1..5e57935 100644
> > --- a/drivers/vdpa/vdpa.c
> > +++ b/drivers/vdpa/vdpa.c
> > @@ -622,13 +622,20 @@ static int vdpa_nl_cmd_dev_add_set_doit(struct 
> > sk_buff *skb, struct genl_info *i
> >           err = PTR_ERR(mdev);
> >           goto err;
> >       }
> > -    if ((config.mask & mdev->config_attr_mask) != config.mask) {
> > +    if (config.mask && (config.mask & mdev->config_attr_mask) == 0) {
> If config.mask is zero than the condition is false even without the 
> change so I don't see why is this change needed.
This way it spews error message when none of the attributes provided by 
user is supported by device.
I can remove the check if improving error message for the other check 
right below, but that is the expected conditional if the same error 
message has to be kept.

> > NL_SET_ERR_MSG_MOD(info->extack,
> >                      "All provided attributes are not supported");
> >           err = -EOPNOTSUPP;
> >           goto err;
> >       }
> >   +    if ((config.mask & mdev->config_attr_mask) != config.mask) {
> > +        NL_SET_ERR_MSG_MOD(info->extack,
> > +                   "Some provided attributes are not supported");
> Changing the message is needed but maybe list the attributes that were 
> specified but are not supported?
If you can live with numeric representing the attributes instead of 
straight attribute string, certainly I can do that. Or else I anticipate 
userspace to query and fail the command rather than present attribute 
strings in kernel.

-Siwei

> > +        err = -EOPNOTSUPP;
> > +        goto err;
> > +    }
> > +
> >       err = mdev->ops->dev_add(mdev, name, &config);
> >   err:
> >       up_write(&vdpa_dev_lock);