* Re: [PATCH] misc: mei: client.c: fix some error code problem in mei_cl_write
2023-11-20 8:53 [PATCH] misc: mei: client.c: fix some error code problem in mei_cl_write Su Hui
@ 2023-11-20 9:14 ` Greg KH
2023-11-20 9:28 ` Su Hui
2023-11-20 15:03 ` Dan Carpenter
2023-11-27 7:10 ` Usyskin, Alexander
2 siblings, 1 reply; 6+ messages in thread
From: Greg KH @ 2023-11-20 9:14 UTC (permalink / raw)
To: Su Hui
Cc: tomas.winkler, arnd, nathan, ndesaulniers, trix,
alexander.usyskin, linux-kernel, llvm, kernel-janitors
On Mon, Nov 20, 2023 at 04:53:45PM +0800, Su Hui wrote:
> Clang static analyzer complains that value stored to 'rets' is never
> read. Remove some useless code, and let 'buf_len = -EOVERFLOW' to make
> sure we can return '-EOVERFLOW'.
>
> mei_msg_hdr_init() return negative error code, rets should be
> 'PTR_ERR(mei_hdr)' rather than '-PTR_ERR(mei_hdr)'.
>
> Fixes: 0cd7c01a60f8 ("mei: add support for mei extended header.")
> Fixes: 8c8d964ce90f ("mei: move hbuf_depth from the mei device to the hw modules")
> Signed-off-by: Su Hui <suhui@nfschina.com>
> ---
> drivers/misc/mei/client.c | 6 ++----
> 1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/misc/mei/client.c b/drivers/misc/mei/client.c
> index 9c8fc87938a7..00dac0a47da0 100644
> --- a/drivers/misc/mei/client.c
> +++ b/drivers/misc/mei/client.c
> @@ -2011,7 +2011,7 @@ ssize_t mei_cl_write(struct mei_cl *cl, struct mei_cl_cb *cb, unsigned long time
>
> mei_hdr = mei_msg_hdr_init(cb);
> if (IS_ERR(mei_hdr)) {
> - rets = -PTR_ERR(mei_hdr);
> + rets = PTR_ERR(mei_hdr);
This looks like a bugfix, while the other changes here are just a normal
cleanup. Can you please split this up into different patches?
thanks,
greg k-h
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] misc: mei: client.c: fix some error code problem in mei_cl_write
2023-11-20 9:14 ` Greg KH
@ 2023-11-20 9:28 ` Su Hui
0 siblings, 0 replies; 6+ messages in thread
From: Su Hui @ 2023-11-20 9:28 UTC (permalink / raw)
To: Greg KH
Cc: tomas.winkler, arnd, nathan, ndesaulniers, trix,
alexander.usyskin, linux-kernel, llvm, kernel-janitors
On 2023/11/20 17:14, Greg KH wrote:
> On Mon, Nov 20, 2023 at 04:53:45PM +0800, Su Hui wrote:
>> Clang static analyzer complains that value stored to 'rets' is never
>> read. Remove some useless code, and let 'buf_len = -EOVERFLOW' to make
>> sure we can return '-EOVERFLOW'.
>>
>> mei_msg_hdr_init() return negative error code, rets should be
>> 'PTR_ERR(mei_hdr)' rather than '-PTR_ERR(mei_hdr)'.
>>
>> Fixes: 0cd7c01a60f8 ("mei: add support for mei extended header.")
>> Fixes: 8c8d964ce90f ("mei: move hbuf_depth from the mei device to the hw modules")
>> Signed-off-by: Su Hui <suhui@nfschina.com>
>> ---
>> drivers/misc/mei/client.c | 6 ++----
>> 1 file changed, 2 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/misc/mei/client.c b/drivers/misc/mei/client.c
>> index 9c8fc87938a7..00dac0a47da0 100644
>> --- a/drivers/misc/mei/client.c
>> +++ b/drivers/misc/mei/client.c
>> @@ -2011,7 +2011,7 @@ ssize_t mei_cl_write(struct mei_cl *cl, struct mei_cl_cb *cb, unsigned long time
>>
>> mei_hdr = mei_msg_hdr_init(cb);
>> if (IS_ERR(mei_hdr)) {
>> - rets = -PTR_ERR(mei_hdr);
>> + rets = PTR_ERR(mei_hdr);
> This looks like a bugfix, while the other changes here are just a normal
> cleanup. Can you please split this up into different patches?
I will split this right now.
Thanks for your suggestion!
Su Hui
>
> thanks,
>
> greg k-h
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] misc: mei: client.c: fix some error code problem in mei_cl_write
2023-11-20 8:53 [PATCH] misc: mei: client.c: fix some error code problem in mei_cl_write Su Hui
2023-11-20 9:14 ` Greg KH
@ 2023-11-20 15:03 ` Dan Carpenter
2023-11-22 3:06 ` Su Hui
2023-11-27 7:10 ` Usyskin, Alexander
2 siblings, 1 reply; 6+ messages in thread
From: Dan Carpenter @ 2023-11-20 15:03 UTC (permalink / raw)
To: Su Hui
Cc: tomas.winkler, arnd, gregkh, nathan, ndesaulniers, trix,
alexander.usyskin, linux-kernel, llvm, kernel-janitors
On Mon, Nov 20, 2023 at 04:53:45PM +0800, Su Hui wrote:
> Clang static analyzer complains that value stored to 'rets' is never
> read. Remove some useless code, and let 'buf_len = -EOVERFLOW' to make
> sure we can return '-EOVERFLOW'.
>
> mei_msg_hdr_init() return negative error code, rets should be
> 'PTR_ERR(mei_hdr)' rather than '-PTR_ERR(mei_hdr)'.
>
> Fixes: 0cd7c01a60f8 ("mei: add support for mei extended header.")
> Fixes: 8c8d964ce90f ("mei: move hbuf_depth from the mei device to the hw modules")
> Signed-off-by: Su Hui <suhui@nfschina.com>
> ---
> drivers/misc/mei/client.c | 6 ++----
> 1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/misc/mei/client.c b/drivers/misc/mei/client.c
> index 9c8fc87938a7..00dac0a47da0 100644
> --- a/drivers/misc/mei/client.c
> +++ b/drivers/misc/mei/client.c
> @@ -2011,7 +2011,7 @@ ssize_t mei_cl_write(struct mei_cl *cl, struct mei_cl_cb *cb, unsigned long time
>
> mei_hdr = mei_msg_hdr_init(cb);
> if (IS_ERR(mei_hdr)) {
> - rets = -PTR_ERR(mei_hdr);
> + rets = PTR_ERR(mei_hdr);
KTODO: write a static checker rule which complains -PTR_ERR()
This might be complicated because there are parts of networking where
we store error codes as positive values. But there is enough context in
this function to create some sort of warning about this code. "Mixing
positive and negative error codes" perhaps?
$ git grep -n '\-PTR_ERR'
block/partitions/core.c:574: disk->disk_name, p, -PTR_ERR(part));
drivers/infiniband/ulp/ipoib/ipoib_multicast.c:291: -PTR_ERR(ah));
drivers/misc/mei/client.c:2014: rets = -PTR_ERR(mei_hdr);
drivers/net/ethernet/intel/igb/igb_main.c:8963: unsigned int xdp_res = -PTR_ERR(skb);
drivers/net/ethernet/intel/igc/igc_main.c:2639: unsigned int xdp_res = -PTR_ERR(skb);
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c:2348: unsigned int xdp_res = -PTR_ERR(skb);
drivers/net/ethernet/stmicro/stmmac/stmmac_main.c:5431: unsigned int xdp_res = -PTR_ERR(skb);
drivers/phy/sunplus/phy-sunplus-usb2.c:278: ret = -PTR_ERR(phy);
drivers/scsi/libfc/fc_elsct.c:86: switch (-PTR_ERR(fp)) {
drivers/scsi/libfc/fc_lport.c:1081: IS_ERR(fp) ? -PTR_ERR(fp) : 0, fc_lport_state(lport),
fs/ext4/indirect.c:1042: ext4_error_inode_block(inode, nr, -PTR_ERR(bh),
fs/jffs2/background.c:48: -PTR_ERR(tsk));
fs/ntfs/dir.c:92: -PTR_ERR(m));
fs/ntfs/dir.c:312: -PTR_ERR(page));
fs/ntfs/dir.c:643: -PTR_ERR(m));
fs/ntfs/dir.c:790: -PTR_ERR(page));
fs/ntfs/index.c:141: -PTR_ERR(m));
fs/ntfs/index.c:268: -PTR_ERR(page));
fs/ntfs/mft.c:162: ntfs_error(ni->vol->sb, "Failed with error code %lu.", -PTR_ERR(m));
fs/ntfs/mft.c:289: "mft record, error code %ld.", -PTR_ERR(m));
net/ipv6/af_inet6.c:852: WRITE_ONCE(sk->sk_err_soft, -PTR_ERR(dst));
net/ipv6/inet6_connection_sock.c:123: WRITE_ONCE(sk->sk_err_soft, -PTR_ERR(dst));
tools/lib/bpf/libbpf.c:10044: errno = -PTR_ERR(ptr);
tools/lib/bpf/libbpf_internal.h:520: errno = -PTR_ERR(ret);
tools/testing/selftests/bpf/prog_tests/btf_dump.c:59: err = -PTR_ERR(btf);
tools/testing/selftests/bpf/prog_tests/sk_lookup.c:483: errno = -PTR_ERR(link);
Quite a few of those were in printks and it might be an opportunity to
use %pe to print the ENOMEM etc strings instead of the number.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] misc: mei: client.c: fix some error code problem in mei_cl_write
2023-11-20 15:03 ` Dan Carpenter
@ 2023-11-22 3:06 ` Su Hui
0 siblings, 0 replies; 6+ messages in thread
From: Su Hui @ 2023-11-22 3:06 UTC (permalink / raw)
To: Dan Carpenter
Cc: tomas.winkler, arnd, gregkh, nathan, ndesaulniers, trix,
alexander.usyskin, linux-kernel, llvm, kernel-janitors
On 2023/11/20 23:03, Dan Carpenter wrote:
> On Mon, Nov 20, 2023 at 04:53:45PM +0800, Su Hui wrote:
>> Clang static analyzer complains that value stored to 'rets' is never
>> read. Remove some useless code, and let 'buf_len = -EOVERFLOW' to make
>> sure we can return '-EOVERFLOW'.
>>
>> mei_msg_hdr_init() return negative error code, rets should be
>> 'PTR_ERR(mei_hdr)' rather than '-PTR_ERR(mei_hdr)'.
>>
>> Fixes: 0cd7c01a60f8 ("mei: add support for mei extended header.")
>> Fixes: 8c8d964ce90f ("mei: move hbuf_depth from the mei device to the hw modules")
>> Signed-off-by: Su Hui <suhui@nfschina.com>
>> ---
>> drivers/misc/mei/client.c | 6 ++----
>> 1 file changed, 2 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/misc/mei/client.c b/drivers/misc/mei/client.c
>> index 9c8fc87938a7..00dac0a47da0 100644
>> --- a/drivers/misc/mei/client.c
>> +++ b/drivers/misc/mei/client.c
>> @@ -2011,7 +2011,7 @@ ssize_t mei_cl_write(struct mei_cl *cl, struct mei_cl_cb *cb, unsigned long time
>>
>> mei_hdr = mei_msg_hdr_init(cb);
>> if (IS_ERR(mei_hdr)) {
>> - rets = -PTR_ERR(mei_hdr);
>> + rets = PTR_ERR(mei_hdr);
> KTODO: write a static checker rule which complains -PTR_ERR()
>
> This might be complicated because there are parts of networking where
> we store error codes as positive values. But there is enough context in
> this function to create some sort of warning about this code. "Mixing
> positive and negative error codes" perhaps?
Maybe add a check in checkpatch.pl is a good idea?
>
> $ git grep -n '\-PTR_ERR'
> block/partitions/core.c:574: disk->disk_name, p, -PTR_ERR(part));
> drivers/infiniband/ulp/ipoib/ipoib_multicast.c:291: -PTR_ERR(ah));
> drivers/misc/mei/client.c:2014: rets = -PTR_ERR(mei_hdr);
> drivers/net/ethernet/intel/igb/igb_main.c:8963: unsigned int xdp_res = -PTR_ERR(skb);
> drivers/net/ethernet/intel/igc/igc_main.c:2639: unsigned int xdp_res = -PTR_ERR(skb);
> drivers/net/ethernet/intel/ixgbe/ixgbe_main.c:2348: unsigned int xdp_res = -PTR_ERR(skb);
> drivers/net/ethernet/stmicro/stmmac/stmmac_main.c:5431: unsigned int xdp_res = -PTR_ERR(skb);
> drivers/phy/sunplus/phy-sunplus-usb2.c:278: ret = -PTR_ERR(phy);
> drivers/scsi/libfc/fc_elsct.c:86: switch (-PTR_ERR(fp)) {
> drivers/scsi/libfc/fc_lport.c:1081: IS_ERR(fp) ? -PTR_ERR(fp) : 0, fc_lport_state(lport),
> fs/ext4/indirect.c:1042: ext4_error_inode_block(inode, nr, -PTR_ERR(bh),
> fs/jffs2/background.c:48: -PTR_ERR(tsk));
> fs/ntfs/dir.c:92: -PTR_ERR(m));
> fs/ntfs/dir.c:312: -PTR_ERR(page));
> fs/ntfs/dir.c:643: -PTR_ERR(m));
> fs/ntfs/dir.c:790: -PTR_ERR(page));
> fs/ntfs/index.c:141: -PTR_ERR(m));
> fs/ntfs/index.c:268: -PTR_ERR(page));
> fs/ntfs/mft.c:162: ntfs_error(ni->vol->sb, "Failed with error code %lu.", -PTR_ERR(m));
> fs/ntfs/mft.c:289: "mft record, error code %ld.", -PTR_ERR(m));
> net/ipv6/af_inet6.c:852: WRITE_ONCE(sk->sk_err_soft, -PTR_ERR(dst));
> net/ipv6/inet6_connection_sock.c:123: WRITE_ONCE(sk->sk_err_soft, -PTR_ERR(dst));
> tools/lib/bpf/libbpf.c:10044: errno = -PTR_ERR(ptr);
> tools/lib/bpf/libbpf_internal.h:520: errno = -PTR_ERR(ret);
> tools/testing/selftests/bpf/prog_tests/btf_dump.c:59: err = -PTR_ERR(btf);
> tools/testing/selftests/bpf/prog_tests/sk_lookup.c:483: errno = -PTR_ERR(link);
>
> Quite a few of those were in printks and it might be an opportunity to
> use %pe to print the ENOMEM etc strings instead of the number.
I will try sending some patch for this.
Thanks for your suggestions.
Su Hui
^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [PATCH] misc: mei: client.c: fix some error code problem in mei_cl_write
2023-11-20 8:53 [PATCH] misc: mei: client.c: fix some error code problem in mei_cl_write Su Hui
2023-11-20 9:14 ` Greg KH
2023-11-20 15:03 ` Dan Carpenter
@ 2023-11-27 7:10 ` Usyskin, Alexander
2 siblings, 0 replies; 6+ messages in thread
From: Usyskin, Alexander @ 2023-11-27 7:10 UTC (permalink / raw)
To: Su Hui, Winkler, Tomas, arnd@arndb.de, gregkh@linuxfoundation.org,
nathan@kernel.org, ndesaulniers@google.com, Rix, Tom
Cc: linux-kernel@vger.kernel.org, llvm@lists.linux.dev,
kernel-janitors@vger.kernel.org
> -----Original Message-----
> From: Su Hui <suhui@nfschina.com>
> Sent: Monday, November 20, 2023 10:54
> To: Winkler, Tomas <tomas.winkler@intel.com>; arnd@arndb.de;
> gregkh@linuxfoundation.org; nathan@kernel.org; ndesaulniers@google.com; Rix,
> Tom <trix@redhat.com>
> Cc: Su Hui <suhui@nfschina.com>; Usyskin, Alexander
> <alexander.usyskin@intel.com>; linux-kernel@vger.kernel.org;
> llvm@lists.linux.dev; kernel-janitors@vger.kernel.org
> Subject: [PATCH] misc: mei: client.c: fix some error code problem in mei_cl_write
>
> Clang static analyzer complains that value stored to 'rets' is never
> read. Remove some useless code, and let 'buf_len = -EOVERFLOW' to make
> sure we can return '-EOVERFLOW'.
>
> mei_msg_hdr_init() return negative error code, rets should be
> 'PTR_ERR(mei_hdr)' rather than '-PTR_ERR(mei_hdr)'.
>
> Fixes: 0cd7c01a60f8 ("mei: add support for mei extended header.")
> Fixes: 8c8d964ce90f ("mei: move hbuf_depth from the mei device to the hw
> modules")
> Signed-off-by: Su Hui <suhui@nfschina.com>
> ---
> drivers/misc/mei/client.c | 6 ++----
> 1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/misc/mei/client.c b/drivers/misc/mei/client.c
> index 9c8fc87938a7..00dac0a47da0 100644
> --- a/drivers/misc/mei/client.c
> +++ b/drivers/misc/mei/client.c
> @@ -2011,7 +2011,7 @@ ssize_t mei_cl_write(struct mei_cl *cl, struct mei_cl_cb
> *cb, unsigned long time
>
> mei_hdr = mei_msg_hdr_init(cb);
> if (IS_ERR(mei_hdr)) {
> - rets = -PTR_ERR(mei_hdr);
> + rets = PTR_ERR(mei_hdr);
> mei_hdr = NULL;
> goto err;
> }
> @@ -2020,19 +2020,17 @@ ssize_t mei_cl_write(struct mei_cl *cl, struct
> mei_cl_cb *cb, unsigned long time
>
> if (rets == 0) {
> cl_dbg(dev, cl, "No flow control credentials: not sending.\n");
> - rets = buf_len;
> goto out;
> }
>
> if (!mei_hbuf_acquire(dev)) {
> cl_dbg(dev, cl, "Cannot acquire the host buffer: not sending.\n");
> - rets = buf_len;
> goto out;
> }
>
> hbuf_slots = mei_hbuf_empty_slots(dev);
> if (hbuf_slots < 0) {
> - rets = -EOVERFLOW;
> + buf_len = -EOVERFLOW;
Here code should go to the error path, not wallpaper over it.
The "goto out;" below should be replaced by "goto err;" instead of above fix.
--
Thanks,
Sasha
> goto out;
> }
>
> --
> 2.30.2
^ permalink raw reply [flat|nested] 6+ messages in thread