[PATCH] net/ice: fix wrong DDP search path

[PATCH] net/ice: fix wrong DDP search path

[Zhichao Zeng]

In the previous implementation, when the user did not enter any value in
"/sys/module/firmware_class/parameters/path", it would incorrectly search
for DDP packages under "/". This commit fixes this issue.

Fixes: 9207f93640a7 ("net/ice: support custom search path for DDP package")

Signed-off-by: Zhichao Zeng <zhichaox.zeng@intel.com>
---
 drivers/net/ice/ice_ethdev.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ice/ice_ethdev.c b/drivers/net/ice/ice_ethdev.c
index d5e94a6685..0705f8e961 100644
--- a/drivers/net/ice/ice_ethdev.c
+++ b/drivers/net/ice/ice_ethdev.c
@@ -1922,8 +1922,11 @@ static int ice_read_customized_path(char *pkg_file, uint16_t buff_len)
 		return -EIO;
 	}
 
-	if (pkg_file[n - 1] == '\n')
+	if (pkg_file[n - 1] == '\n') {
 		n--;
+		if (n == 0)
+			return -EINVAL;
+	}
 
 	pkg_file[n] = '\0';
 
-- 
2.34.1

Re: [PATCH] net/ice: fix wrong DDP search path

[Bruce Richardson]

On Fri, Nov 01, 2024 at 04:44:43PM +0800, Zhichao Zeng wrote:
> In the previous implementation, when the user did not enter any value in
> "/sys/module/firmware_class/parameters/path", it would incorrectly search
> for DDP packages under "/". This commit fixes this issue.
> 
> Fixes: 9207f93640a7 ("net/ice: support custom search path for DDP package")
> 
> Signed-off-by: Zhichao Zeng <zhichaox.zeng@intel.com>
> ---
>  drivers/net/ice/ice_ethdev.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ice/ice_ethdev.c b/drivers/net/ice/ice_ethdev.c
> index d5e94a6685..0705f8e961 100644
> --- a/drivers/net/ice/ice_ethdev.c
> +++ b/drivers/net/ice/ice_ethdev.c
> @@ -1922,8 +1922,11 @@ static int ice_read_customized_path(char *pkg_file, uint16_t buff_len)
>  		return -EIO;
>  	}
>  
> -	if (pkg_file[n - 1] == '\n')
> +	if (pkg_file[n - 1] == '\n') {
>  		n--;
> +		if (n == 0)
> +			return -EINVAL;
> +	}
>  
>  	pkg_file[n] = '\0';
>  

May I suggest a slightly alternative fix, that I think it a little shorter
and neater (assuming it works - if not, let me know.)

Rather than adding an explicit check for n==0 and returning error, I think
we can instead change the return value of the function to be the length of
the data read. So at line 1931 "return 0" we can change that to "return n".
Then at the call site for the function we can change:

	if (ice_read_customized_path(....) == 0) {

to 
	if (ice_read_customized_path(....) > 0) {

What do you think?

/Bruce

PS: if you do take this approach, we can also slightly shorten the function
by changing/removing the block:

	if (n == 0) {
		close(fp);
		return -EIO;
	}
	... /* length adjust and zeroing */
	return n;

to be an inverted check with the length adjustment and zeroing inside it:
	if (n > 0){
		if (pkg_file[n -1] == '\n')
			n--;
		pkg_file[n] = '\0';
	}
	close(fp);
	return n;

That gives us a zero return value too in case of reading zero bytes. It
also handles the currently unhandled case of read returning < 0.

RE: [PATCH] net/ice: fix wrong DDP search path

[Zeng, ZhichaoX]

Hi Bruce,

Thanks for your suggestions, will send v2 patch.

Regards
Zhichao

> -----Original Message-----
> From: Richardson, Bruce <bruce.richardson@intel.com>
> Sent: Friday, November 1, 2024 5:36 PM
> To: Zeng, ZhichaoX <zhichaox.zeng@intel.com>
> Cc: dev@dpdk.org; Burakov, Anatoly <anatoly.burakov@intel.com>
> Subject: Re: [PATCH] net/ice: fix wrong DDP search path
> 
> On Fri, Nov 01, 2024 at 04:44:43PM +0800, Zhichao Zeng wrote:
> > In the previous implementation, when the user did not enter any value
> > in "/sys/module/firmware_class/parameters/path", it would incorrectly
> > search for DDP packages under "/". This commit fixes this issue.
> >
> > Fixes: 9207f93640a7 ("net/ice: support custom search path for DDP
> > package")
> >
> > Signed-off-by: Zhichao Zeng <zhichaox.zeng@intel.com>
> > ---
> >  drivers/net/ice/ice_ethdev.c | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/ice/ice_ethdev.c
> > b/drivers/net/ice/ice_ethdev.c index d5e94a6685..0705f8e961 100644
> > --- a/drivers/net/ice/ice_ethdev.c
> > +++ b/drivers/net/ice/ice_ethdev.c
> > @@ -1922,8 +1922,11 @@ static int ice_read_customized_path(char
> *pkg_file, uint16_t buff_len)
> >  		return -EIO;
> >  	}
> >
> > -	if (pkg_file[n - 1] == '\n')
> > +	if (pkg_file[n - 1] == '\n') {
> >  		n--;
> > +		if (n == 0)
> > +			return -EINVAL;
> > +	}
> >
> >  	pkg_file[n] = '\0';
> >
> 
> May I suggest a slightly alternative fix, that I think it a little shorter and neater
> (assuming it works - if not, let me know.)
> 
> Rather than adding an explicit check for n==0 and returning error, I think we
> can instead change the return value of the function to be the length of the
> data read. So at line 1931 "return 0" we can change that to "return n".
> Then at the call site for the function we can change:
> 
> 	if (ice_read_customized_path(....) == 0) {
> 
> to
> 	if (ice_read_customized_path(....) > 0) {
> 
> What do you think?
> 
> /Bruce
> 
> PS: if you do take this approach, we can also slightly shorten the function by
> changing/removing the block:
> 
> 	if (n == 0) {
> 		close(fp);
> 		return -EIO;
> 	}
> 	... /* length adjust and zeroing */
> 	return n;
> 
> to be an inverted check with the length adjustment and zeroing inside it:
> 	if (n > 0){
> 		if (pkg_file[n -1] == '\n')
> 			n--;
> 		pkg_file[n] = '\0';
> 	}
> 	close(fp);
> 	return n;
> 
> That gives us a zero return value too in case of reading zero bytes. It also
> handles the currently unhandled case of read returning < 0.

[PATCH v2] net/ice: fix wrong DDP search path

[Zhichao Zeng]

In the previous implementation, when the user did not enter any value in
"/sys/module/firmware_class/parameters/path", it would incorrectly search
for DDP packages under "/".

This commit fixes this issue.

Fixes: 9207f93640a7 ("net/ice: support custom search path for DDP package")

Signed-off-by: Zhichao Zeng <zhichaox.zeng@intel.com>

v2: change return code
---
 drivers/net/ice/ice_ethdev.c | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/drivers/net/ice/ice_ethdev.c b/drivers/net/ice/ice_ethdev.c
index d5e94a6685..cf06ac58ce 100644
--- a/drivers/net/ice/ice_ethdev.c
+++ b/drivers/net/ice/ice_ethdev.c
@@ -1917,18 +1917,14 @@ static int ice_read_customized_path(char *pkg_file, uint16_t buff_len)
 	}
 
 	n = read(fp, pkg_file, buff_len - 1);
-	if (n == 0) {
-		close(fp);
-		return -EIO;
+	if (n > 0) {
+		if (pkg_file[n - 1] == '\n')
+			n--;
+		pkg_file[n] = '\0';
 	}
 
-	if (pkg_file[n - 1] == '\n')
-		n--;
-
-	pkg_file[n] = '\0';
-
 	close(fp);
-	return 0;
+	return n;
 }
 
 int ice_load_pkg(struct ice_adapter *adapter, bool use_dsn, uint64_t dsn)
@@ -1956,7 +1952,7 @@ int ice_load_pkg(struct ice_adapter *adapter, bool use_dsn, uint64_t dsn)
 	snprintf(opt_ddp_filename, ICE_MAX_PKG_FILENAME_SIZE,
 		"ice-%016" PRIx64 ".pkg", dsn);
 
-	if (ice_read_customized_path(customized_path, ICE_MAX_PKG_FILENAME_SIZE) == 0) {
+	if (ice_read_customized_path(customized_path, ICE_MAX_PKG_FILENAME_SIZE) > 0) {
 		if (use_dsn) {
 			snprintf(pkg_file, RTE_DIM(pkg_file), "%s/%s",
 					customized_path, opt_ddp_filename);
-- 
2.34.1

Re: [PATCH v2] net/ice: fix wrong DDP search path

[Bruce Richardson]

On Mon, Nov 04, 2024 at 06:09:34PM +0800, Zhichao Zeng wrote:
> In the previous implementation, when the user did not enter any value in
> "/sys/module/firmware_class/parameters/path", it would incorrectly search
> for DDP packages under "/".
> 
> This commit fixes this issue.
> 
> Fixes: 9207f93640a7 ("net/ice: support custom search path for DDP package")
> 
> Signed-off-by: Zhichao Zeng <zhichaox.zeng@intel.com>
> 
> v2: change return code
> ---
>  drivers/net/ice/ice_ethdev.c | 16 ++++++----------
>  1 file changed, 6 insertions(+), 10 deletions(-)
> 
Acked-by: Bruce Richardson <bruce.richardson@intel.com>

Patch applied to dpdk-next-net-intel.

Thanks,
/Bruce

RE: [PATCH v2] net/ice: fix wrong DDP search path

[Li, HongboX]

> -----Original Message-----
> From: Zhichao Zeng <zhichaox.zeng@intel.com>
> Sent: Monday, November 4, 2024 6:10 PM
> To: dev@dpdk.org
> Cc: Zeng, ZhichaoX <zhichaox.zeng@intel.com>; Richardson, Bruce
> <bruce.richardson@intel.com>; Burakov, Anatoly
> <anatoly.burakov@intel.com>
> Subject: [PATCH v2] net/ice: fix wrong DDP search path
> 
> In the previous implementation, when the user did not enter any value in
> "/sys/module/firmware_class/parameters/path", it would incorrectly search
> for DDP packages under "/".
> 
> This commit fixes this issue.
> 
> Fixes: 9207f93640a7 ("net/ice: support custom search path for DDP
> package")
> 
> Signed-off-by: Zhichao Zeng <zhichaox.zeng@intel.com>
> 
> v2: change return code
> ---
Tested-by: Li, Hongbo <hongbox.li@intel.com>