Re: [PATCH net-next v2 6/9] devlink: support directly reading from region memory

[Jacob Keller <jacob.e.keller@intel.com>]

On 11/24/2022 1:05 AM, Jiri Pirko wrote:
> Wed, Nov 23, 2022 at 09:38:31PM CET, jacob.e.keller@intel.com wrote:
> 
> Hmm. Being pedantic here, it changes userspace expectations:
> current - message without snapshotid is errored out
> new - message without snapshotid is happily processed
> 

Yea. I guess I'm thinking more from an interactive application 
standpoint this wouldn't be a problem but from a scripted setup it might 
do something weird.

> I can imagine some obscure userspace app depending on this behaviour,
> in theory.

Hmm.

> 
> Safe would be to add new NLA_FLAG attr DEVLINK_ATTR_REGION_DIRECT or
> something that would indicate userspace is interested in direct read.
> Also, the userspace would know right away it this new functionality
> is supported or not by the kernel.
> 

The advantage of being able to see that such a feature exists is good.

I can rework this to add an attribute.

> 
> 
>> +		if (!region->ops->read) {
>> +			NL_SET_ERR_MSG(cb->extack, "requested region does not support direct read");
>> +			err = -EOPNOTSUPP;
>> +			goto out_unlock;
>> +		}
>> +		if (port)
>> +			region_cb = &devlink_region_port_direct_fill;
>> +		else
>> +			region_cb = &devlink_region_direct_fill;
>> +		region_cb_priv = region;
>> +	} else {
>> +		struct devlink_snapshot *snapshot;
>> +		u32 snapshot_id;
>> +
>> +		snapshot_id = nla_get_u32(snapshot_attr);
>> +		snapshot = devlink_region_snapshot_get_by_id(region, snapshot_id);
>> +		if (!snapshot) {
>> +			NL_SET_ERR_MSG_ATTR(cb->extack, snapshot_attr, "requested snapshot does not exist");
>> +			err = -EINVAL;
>> +			goto out_unlock;
>> +		}
>> +		region_cb = &devlink_region_snapshot_fill;
>> +		region_cb_priv = snapshot;
>> 	}
>>
>> 	if (attrs[DEVLINK_ATTR_REGION_CHUNK_ADDR] &&
>> @@ -6633,9 +6665,9 @@ static int devlink_nl_cmd_region_read_dumpit(struct sk_buff *skb,
>> 		goto nla_put_failure;
>> 	}
>>
>> -	err = devlink_nl_region_read_fill(skb, &devlink_region_snapshot_fill,
>> -					  snapshot, start_offset, end_offset,
>> -					  &ret_offset, cb->extack);
>> +	err = devlink_nl_region_read_fill(skb, region_cb, region_cb_priv,
>> +					  start_offset, end_offset, &ret_offset,
>> +					  cb->extack);
>>
>> 	if (err && err != -EMSGSIZE)
>> 		goto nla_put_failure;
>> -- 
>> 2.38.1.420.g319605f8f00e
>>