Si-Wei Liu
2023-Jan-31 22:51 UTC
[PATCH 1/6] vdpa: fix improper error message when adding vdpa dev
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 at oracle.com> >> Acked-by: Jason Wang <jasowang at redhat.com> >> --- >> ? 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);