netdev - Re: [PATCH v5 net-next 2/8] sfc: add devlink info support for ef100

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <DM6PR12MB4202E78CB7CB3BE13817B782C1DB9@DM6PR12MB4202.namprd12.prod.outlook.com>
Date:   Tue, 7 Feb 2023 17:24:05 +0000
From:   "Lucero Palau, Alejandro" <alejandro.lucero-palau@....com>
To:     "Lucero Palau, Alejandro" <alejandro.lucero-palau@....com>,
        Jiri Pirko <jiri@...nulli.us>
CC:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linux-net-drivers (AMD-Xilinx)" <linux-net-drivers@....com>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "kuba@...nel.org" <kuba@...nel.org>,
        "pabeni@...hat.com" <pabeni@...hat.com>,
        "edumazet@...gle.com" <edumazet@...gle.com>,
        "habetsm.xilinx@...il.com" <habetsm.xilinx@...il.com>,
        "ecree.xilinx@...il.com" <ecree.xilinx@...il.com>,
        "linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>,
        "corbet@....net" <corbet@....net>,
        "jiri@...dia.com" <jiri@...dia.com>
Subject: Re: [PATCH v5 net-next 2/8] sfc: add devlink info support for ef100


On 2/7/23 15:10, Lucero Palau, Alejandro wrote:
> On 2/7/23 14:58, Jiri Pirko wrote:
>> Tue, Feb 07, 2023 at 03:42:45PM CET, alejandro.lucero-palau@....com wrote:
>>> On 2/2/23 11:58, Jiri Pirko wrote:
>>>> Thu, Feb 02, 2023 at 12:14:17PM CET, alejandro.lucero-palau@....com wrote:
>>>>> From: Alejandro Lucero <alejandro.lucero-palau@....com>
>>>>>
>>>>> Support for devlink info command.
>>>> You are quite brief for couple hundred line patch. Care to shed some
>>>> more details for the reader? Also, use imperative mood (applies to the
>>>> rest of the pathes)
>>>>
>>>> [...]
>>>>
>>> OK. I'll be more talkative and imperative here.
>>>
>>>>> +static int efx_devlink_info_get(struct devlink *devlink,
>>>>> +				struct devlink_info_req *req,
>>>>> +				struct netlink_ext_ack *extack)
>>>>> +{
>>>>> +	struct efx_devlink *devlink_private = devlink_priv(devlink);
>>>>> +	struct efx_nic *efx = devlink_private->efx;
>>>>> +	char msg[NETLINK_MAX_FMTMSG_LEN];
>>>>> +	int errors_reported = 0;
>>>>> +	int rc;
>>>>> +
>>>>> +	/* Several different MCDI commands are used. We report first error
>>>>> +	 * through extack along with total number of errors. Specific error
>>>>> +	 * information via system messages.
>>>>> +	 */
>>>>> +	rc = efx_devlink_info_board_cfg(efx, req);
>>>>> +	if (rc) {
>>>>> +		sprintf(msg, "Getting board info failed");
>>>>> +		errors_reported++;
>>>>> +	}
>>>>> +	rc = efx_devlink_info_stored_versions(efx, req);
>>>>> +	if (rc) {
>>>>> +		if (!errors_reported)
>>>>> +			sprintf(msg, "Getting stored versions failed");
>>>>> +		errors_reported += rc;
>>>>> +	}
>>>>> +	rc = efx_devlink_info_running_versions(efx, req);
>>>>> +	if (rc) {
>>>>> +		if (!errors_reported)
>>>>> +			sprintf(msg, "Getting board info failed");
>>>>> +		errors_reported++;
>>>> Under which circumstances any of the errors above happen? Is it a common
>>>> thing? Or is it result of some fatal event?
>>> They are not common at all. If any of those happen, it is a bad sign,
>>> and it is more than likely there are more than one because something is
>>> not working properly. That is the reason I only report first error found
>>> plus the total number of errors detected.
>>>
>>>
>>>> You treat it like it is quite common, which seems very odd to me.
>>>> If they are rare, just return error right away to the caller.
>>> Well, that is done now. And as I say, I'm not reporting all but just the
>>> first one, mainly because the buffer limitation with NETLINK_MAX_FMTMSG_LEN.
>>>
>>> If errors trigger, a more complete information will appear in system
>>> messages, so that is the reason with:
>>>
>>> +               NL_SET_ERR_MSG_FMT(extack,
>>> +                                  "%s. %d total errors. Check system messages",
>>> +                                  msg, errors_reported);
>>>
>>> I guess you are concerned with the extack report being overwhelmed, but
>>> I do not think that is the case.
>> No, I'm wondering why you just don't put error message into exack and
>> return -ESOMEERROR right away.
> Well, I thought the idea was to give more information to user space
> about the problem.
>
> Previous patchsets were not reporting any error nor error information
> through extack. Now we have both.


Just trying to make more sense of this.

Because that limit with NETLINK_MAX_FMTMSG_LEN, what I think is big 
enough, some control needs to be taken about what to report. It could be 
just to write the buffer with the last error and report that last one 
only, with no need of keeping total errors count. But I felt once we 
handle any error, reporting that extra info about the total errors 
detected should not be a problem at all, even if it is an unlikely 
situation.

BTW, I said we were reporting both, the error and the extack error 
message, but I've realized the function was not returning any error but 
always 0, so I'll fix that.


>>>>> +	}
>>>>> +
>>>>> +	if (errors_reported)
>>>>> +		NL_SET_ERR_MSG_FMT(extack,
>>>>> +				   "%s. %d total errors. Check system messages",
>>>>> +				   msg, errors_reported);
>>>>> +	return 0;
>>>>> +}
>>>>> +
>>>>> static const struct devlink_ops sfc_devlink_ops = {
>>>>> +	.info_get			= efx_devlink_info_get,
>>>>> };
>>>> [...]