[PATCH v1 0/3] Bluetooth: fix recv_buf() return value

[PATCH v1 0/3] Bluetooth: fix recv_buf() return value

[Francesco Dolcini]

From: Francesco Dolcini <francesco.dolcini@toradex.com>

Serdev recv_buf() callback is supposed to return the amount of bytes consumed, therefore an int in between 0 and count.

Do not return negative number in case of issue, just print an error and return count. This fixes a WARN in ttyport_receive_buf().

In addition to that a small cleanup patch is added on btnxpuart to remove a useless assignment.

Francesco Dolcini (3):
  Bluetooth: btnxpuart: fix recv_buf() return value
  Bluetooth: btmtkuart: fix recv_buf() return value
  Bluetooth: btnxpuart: remove useless assignment

 drivers/bluetooth/btmtkuart.c | 11 +++--------
 drivers/bluetooth/btnxpuart.c |  8 +++-----
 2 files changed, 6 insertions(+), 13 deletions(-)

-- 
2.25.1

[PATCH v1 1/3] Bluetooth: btnxpuart: fix recv_buf() return value

[Francesco Dolcini]

From: Francesco Dolcini <francesco.dolcini@toradex.com>

Serdev recv_buf() callback is supposed to return the amount of bytes
consumed, therefore an int in between 0 and count.

Do not return negative number in case of issue, just print an error and
return count.  This fixes a WARN in ttyport_receive_buf().

[    9.962266] Bluetooth: hci0: Frame reassembly failed (-84)
[    9.972939] ------------[ cut here ]------------
[    9.977922] serial serial0: receive_buf returns -84 (count = 6)
[    9.994857] WARNING: CPU: 0 PID: 37 at drivers/tty/serdev/serdev-ttyport.c:37 ttyport_receive_buf+0xd8/0xf8
[   10.004840] Modules linked in: mwifiex_sdio(+) mwifiex snd_soc_simple_card crct10dif_ce cfg80211 snd_soc_simple_card_utils k3_j72xx_bandgap rti_wdt rtc_ti_k3 btnxpuart bluetooth sa2ul ecdh_generic ecc sha256_generic tidss rfkill libsha256 drm_dma_helper snd_soc_davinci_mcasp authenc omap_mailbox snd_soc_ti_udma snd_soc_ti_edma snd_soc_ti_sdma atmel_mxt_ts ina2xx snd_soc_nau8822 ti_sn65dsi83 tc358768 ti_ads1015 tps65219_pwrbutton at24 m_can_platform industrialio_triggered_buffer drm_kms_helper m_can kfifo_buf rtc_ds1307 lm75 pwm_tiehrpwm can_dev spi_omap2_mcspi panel_lvds pwm_bl libcomposite fuse drm backlight ipv6
[   10.059984] CPU: 0 PID: 37 Comm: kworker/u4:2 Not tainted 6.7.0-rc2-00147-gf1a09972a45a #1
[   10.071793] Hardware name: Toradex Verdin AM62 WB on Verdin Development Board (DT)
[   10.082898] Workqueue: events_unbound flush_to_ldisc
[   10.091345] pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
[   10.101820] pc : ttyport_receive_buf+0xd8/0xf8
[   10.109712] lr : ttyport_receive_buf+0xd8/0xf8
[   10.117581] sp : ffff800082b9bd20
[   10.124202] x29: ffff800082b9bd20 x28: ffff00000000ee05 x27: ffff0000002f21c0
[   10.134735] x26: ffff000002931820 x25: 61c8864680b583eb x24: ffff0000002f21b8
[   10.145209] x23: ffff00000026e740 x22: ffff0000002f21e0 x21: ffffffffffffffac
[   10.155686] x20: ffff000000da5c00 x19: 0000000000000006 x18: 0000000000000000
[   10.166178] x17: ffff7fffbe0e7000 x16: ffff800080000000 x15: 000039966db1c650
[   10.176564] x14: 000000000000022c x13: 000000000000022c x12: 0000000000000000
[   10.186979] x11: 000000000000000a x10: 0000000000000a60 x9 : ffff800082b9bb80
[   10.197352] x8 : ffff00000026f200 x7 : ffff00003fd90080 x6 : 00000000000022e5
[   10.207680] x5 : 00000000410fd030 x4 : 0000000000c0000e x3 : ffff7fffbe0e7000
[   10.218051] x2 : 0000000000000002 x1 : 0000000000000000 x0 : 0000000000000000
[   10.228393] Call trace:
[   10.233989]  ttyport_receive_buf+0xd8/0xf8
[   10.241224]  flush_to_ldisc+0xbc/0x1a4
[   10.248117]  process_scheduled_works+0x16c/0x28c
[   10.255851]  worker_thread+0x16c/0x2e0
[   10.262673]  kthread+0x11c/0x128
[   10.268953]  ret_from_fork+0x10/0x20
[   10.275460] ---[ end trace 0000000000000000 ]---

Closes: https://lore.kernel.org/all/ZWEIhcUXfutb5SY6@francesco-nb.int.toradex.com/
Fixes: 689ca16e5232 ("Bluetooth: NXP: Add protocol support for NXP Bluetooth chipsets")
Signed-off-by: Francesco Dolcini <francesco.dolcini@toradex.com>
---
 drivers/bluetooth/btnxpuart.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/bluetooth/btnxpuart.c b/drivers/bluetooth/btnxpuart.c
index b7e66b7ac570..951fe3014a3f 100644
--- a/drivers/bluetooth/btnxpuart.c
+++ b/drivers/bluetooth/btnxpuart.c
@@ -1276,11 +1276,10 @@ static int btnxpuart_receive_buf(struct serdev_device *serdev, const u8 *data,
 	if (IS_ERR(nxpdev->rx_skb)) {
 		int err = PTR_ERR(nxpdev->rx_skb);
 		/* Safe to ignore out-of-sync bootloader signatures */
-		if (is_fw_downloading(nxpdev))
-			return count;
-		bt_dev_err(nxpdev->hdev, "Frame reassembly failed (%d)", err);
+		if (!is_fw_downloading(nxpdev))
+			bt_dev_err(nxpdev->hdev, "Frame reassembly failed (%d)", err);
 		nxpdev->rx_skb = NULL;
-		return err;
+		return count;
 	}
 	if (!is_fw_downloading(nxpdev))
 		nxpdev->hdev->stat.byte_rx += count;
-- 
2.25.1

Re: [PATCH v1 1/3] Bluetooth: btnxpuart: fix recv_buf() return value

[Greg KH]

On Mon, Nov 27, 2023 at 08:14:06PM +0100, Francesco Dolcini wrote:
> From: Francesco Dolcini <francesco.dolcini@toradex.com>
> 
> Serdev recv_buf() callback is supposed to return the amount of bytes
> consumed, therefore an int in between 0 and count.
> 
> Do not return negative number in case of issue, just print an error and
> return count.  This fixes a WARN in ttyport_receive_buf().
> 
> [    9.962266] Bluetooth: hci0: Frame reassembly failed (-84)
> [    9.972939] ------------[ cut here ]------------
> [    9.977922] serial serial0: receive_buf returns -84 (count = 6)
> [    9.994857] WARNING: CPU: 0 PID: 37 at drivers/tty/serdev/serdev-ttyport.c:37 ttyport_receive_buf+0xd8/0xf8
> [   10.004840] Modules linked in: mwifiex_sdio(+) mwifiex snd_soc_simple_card crct10dif_ce cfg80211 snd_soc_simple_card_utils k3_j72xx_bandgap rti_wdt rtc_ti_k3 btnxpuart bluetooth sa2ul ecdh_generic ecc sha256_generic tidss rfkill libsha256 drm_dma_helper snd_soc_davinci_mcasp authenc omap_mailbox snd_soc_ti_udma snd_soc_ti_edma snd_soc_ti_sdma atmel_mxt_ts ina2xx snd_soc_nau8822 ti_sn65dsi83 tc358768 ti_ads1015 tps65219_pwrbutton at24 m_can_platform industrialio_triggered_buffer drm_kms_helper m_can kfifo_buf rtc_ds1307 lm75 pwm_tiehrpwm can_dev spi_omap2_mcspi panel_lvds pwm_bl libcomposite fuse drm backlight ipv6
> [   10.059984] CPU: 0 PID: 37 Comm: kworker/u4:2 Not tainted 6.7.0-rc2-00147-gf1a09972a45a #1
> [   10.071793] Hardware name: Toradex Verdin AM62 WB on Verdin Development Board (DT)
> [   10.082898] Workqueue: events_unbound flush_to_ldisc
> [   10.091345] pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> [   10.101820] pc : ttyport_receive_buf+0xd8/0xf8
> [   10.109712] lr : ttyport_receive_buf+0xd8/0xf8
> [   10.117581] sp : ffff800082b9bd20
> [   10.124202] x29: ffff800082b9bd20 x28: ffff00000000ee05 x27: ffff0000002f21c0
> [   10.134735] x26: ffff000002931820 x25: 61c8864680b583eb x24: ffff0000002f21b8
> [   10.145209] x23: ffff00000026e740 x22: ffff0000002f21e0 x21: ffffffffffffffac
> [   10.155686] x20: ffff000000da5c00 x19: 0000000000000006 x18: 0000000000000000
> [   10.166178] x17: ffff7fffbe0e7000 x16: ffff800080000000 x15: 000039966db1c650
> [   10.176564] x14: 000000000000022c x13: 000000000000022c x12: 0000000000000000
> [   10.186979] x11: 000000000000000a x10: 0000000000000a60 x9 : ffff800082b9bb80
> [   10.197352] x8 : ffff00000026f200 x7 : ffff00003fd90080 x6 : 00000000000022e5
> [   10.207680] x5 : 00000000410fd030 x4 : 0000000000c0000e x3 : ffff7fffbe0e7000
> [   10.218051] x2 : 0000000000000002 x1 : 0000000000000000 x0 : 0000000000000000
> [   10.228393] Call trace:
> [   10.233989]  ttyport_receive_buf+0xd8/0xf8
> [   10.241224]  flush_to_ldisc+0xbc/0x1a4
> [   10.248117]  process_scheduled_works+0x16c/0x28c
> [   10.255851]  worker_thread+0x16c/0x2e0
> [   10.262673]  kthread+0x11c/0x128
> [   10.268953]  ret_from_fork+0x10/0x20
> [   10.275460] ---[ end trace 0000000000000000 ]---
> 
> Closes: https://lore.kernel.org/all/ZWEIhcUXfutb5SY6@francesco-nb.int.toradex.com/
> Fixes: 689ca16e5232 ("Bluetooth: NXP: Add protocol support for NXP Bluetooth chipsets")
> Signed-off-by: Francesco Dolcini <francesco.dolcini@toradex.com>
> ---
>  drivers/bluetooth/btnxpuart.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 

Hi,

This is the friendly patch-bot of Greg Kroah-Hartman.  You have sent him
a patch that has triggered this response.  He used to manually respond
to these common problems, but in order to save his sanity (he kept
writing the same thing over and over, yet to different people), I was
created.  Hopefully you will not take offence and will fix the problem
in your patch and resubmit it so that it can be accepted into the Linux
kernel tree.

You are receiving this message because of the following common error(s)
as indicated below:

- You have marked a patch with a "Fixes:" tag for a commit that is in an
  older released kernel, yet you do not have a cc: stable line in the
  signed-off-by area at all, which means that the patch will not be
  applied to any older kernel releases.  To properly fix this, please
  follow the documented rules in the
  Documentation/process/stable-kernel-rules.rst file for how to resolve
  this.

If you wish to discuss this problem further, or you have questions about
how to resolve this issue, please feel free to respond to this email and
Greg will reply once he has dug out from the pending patches received
from other developers.

thanks,

greg k-h's patch email bot

RE: Bluetooth: fix recv_buf() return value

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 3684 bytes --]

This is automated email and please do not reply to this email!

Dear submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=804631

---Test result---

Test Summary:
CheckPatch                    PASS      1.76 seconds
GitLint                       FAIL      1.22 seconds
SubjectPrefix                 PASS      0.35 seconds
BuildKernel                   PASS      27.45 seconds
CheckAllWarning               PASS      30.18 seconds
CheckSparse                   PASS      35.86 seconds
CheckSmatch                   PASS      99.01 seconds
BuildKernel32                 PASS      26.57 seconds
TestRunnerSetup               PASS      416.78 seconds
TestRunner_l2cap-tester       PASS      23.18 seconds
TestRunner_iso-tester         PASS      45.81 seconds
TestRunner_bnep-tester        PASS      7.03 seconds
TestRunner_mgmt-tester        PASS      170.01 seconds
TestRunner_rfcomm-tester      PASS      10.95 seconds
TestRunner_sco-tester         PASS      14.68 seconds
TestRunner_ioctl-tester       PASS      12.14 seconds
TestRunner_mesh-tester        PASS      8.83 seconds
TestRunner_smp-tester         PASS      9.88 seconds
TestRunner_userchan-tester    PASS      7.36 seconds
IncrementalBuild              PASS      34.37 seconds

Details
##############################
Test: GitLint - FAIL
Desc: Run gitlint
Output:
[v1,1/3] Bluetooth: btnxpuart: fix recv_buf() return value

WARNING: I3 - ignore-body-lines: gitlint will be switching from using Python regex 'match' (match beginning) to 'search' (match anywhere) semantics. Please review your ignore-body-lines.regex option accordingly. To remove this warning, set general.regex-style-search=True. More details: https://jorisroovers.github.io/gitlint/configuration/#regex-style-search
14: B1 Line exceeds max length (110>80): "[    9.994857] WARNING: CPU: 0 PID: 37 at drivers/tty/serdev/serdev-ttyport.c:37 ttyport_receive_buf+0xd8/0xf8"
15: B1 Line exceeds max length (624>80): "[   10.004840] Modules linked in: mwifiex_sdio(+) mwifiex snd_soc_simple_card crct10dif_ce cfg80211 snd_soc_simple_card_utils k3_j72xx_bandgap rti_wdt rtc_ti_k3 btnxpuart bluetooth sa2ul ecdh_generic ecc sha256_generic tidss rfkill libsha256 drm_dma_helper snd_soc_davinci_mcasp authenc omap_mailbox snd_soc_ti_udma snd_soc_ti_edma snd_soc_ti_sdma atmel_mxt_ts ina2xx snd_soc_nau8822 ti_sn65dsi83 tc358768 ti_ads1015 tps65219_pwrbutton at24 m_can_platform industrialio_triggered_buffer drm_kms_helper m_can kfifo_buf rtc_ds1307 lm75 pwm_tiehrpwm can_dev spi_omap2_mcspi panel_lvds pwm_bl libcomposite fuse drm backlight ipv6"
16: B1 Line exceeds max length (93>80): "[   10.059984] CPU: 0 PID: 37 Comm: kworker/u4:2 Not tainted 6.7.0-rc2-00147-gf1a09972a45a #1"
17: B1 Line exceeds max length (85>80): "[   10.071793] Hardware name: Toradex Verdin AM62 WB on Verdin Development Board (DT)"
42: B1 Line exceeds max length (82>80): "Closes: https://lore.kernel.org/all/ZWEIhcUXfutb5SY6@francesco-nb.int.toradex.com/"
[v1,2/3] Bluetooth: btmtkuart: fix recv_buf() return value

WARNING: I3 - ignore-body-lines: gitlint will be switching from using Python regex 'match' (match beginning) to 'search' (match anywhere) semantics. Please review your ignore-body-lines.regex option accordingly. To remove this warning, set general.regex-style-search=True. More details: https://jorisroovers.github.io/gitlint/configuration/#regex-style-search
11: B1 Line exceeds max length (82>80): "Link: https://lore.kernel.org/all/087be419-ec6b-47ad-851a-5e1e3ea5cfcc@kernel.org/"


---
Regards,
Linux Bluetooth

Re: [PATCH v1 1/3] Bluetooth: btnxpuart: fix recv_buf() return value

[Jiri Slaby]

On 27. 11. 23, 20:14, Francesco Dolcini wrote:
> From: Francesco Dolcini <francesco.dolcini@toradex.com>
> 
> Serdev recv_buf() callback is supposed to return the amount of bytes
> consumed, therefore an int in between 0 and count.
> 
> Do not return negative number in case of issue, just print an error and
> return count.  This fixes a WARN in ttyport_receive_buf().
> 
> [    9.962266] Bluetooth: hci0: Frame reassembly failed (-84)
> [    9.972939] ------------[ cut here ]------------
> [    9.977922] serial serial0: receive_buf returns -84 (count = 6)
> [    9.994857] WARNING: CPU: 0 PID: 37 at drivers/tty/serdev/serdev-ttyport.c:37 ttyport_receive_buf+0xd8/0xf8
> [   10.004840] Modules linked in: mwifiex_sdio(+) mwifiex snd_soc_simple_card crct10dif_ce cfg80211 snd_soc_simple_card_utils k3_j72xx_bandgap rti_wdt rtc_ti_k3 btnxpuart bluetooth sa2ul ecdh_generic ecc sha256_generic tidss rfkill libsha256 drm_dma_helper snd_soc_davinci_mcasp authenc omap_mailbox snd_soc_ti_udma snd_soc_ti_edma snd_soc_ti_sdma atmel_mxt_ts ina2xx snd_soc_nau8822 ti_sn65dsi83 tc358768 ti_ads1015 tps65219_pwrbutton at24 m_can_platform industrialio_triggered_buffer drm_kms_helper m_can kfifo_buf rtc_ds1307 lm75 pwm_tiehrpwm can_dev spi_omap2_mcspi panel_lvds pwm_bl libcomposite fuse drm backlight ipv6
> [   10.059984] CPU: 0 PID: 37 Comm: kworker/u4:2 Not tainted 6.7.0-rc2-00147-gf1a09972a45a #1
> [   10.071793] Hardware name: Toradex Verdin AM62 WB on Verdin Development Board (DT)
> [   10.082898] Workqueue: events_unbound flush_to_ldisc
> [   10.091345] pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> [   10.101820] pc : ttyport_receive_buf+0xd8/0xf8
> [   10.109712] lr : ttyport_receive_buf+0xd8/0xf8

 From here:

> [   10.117581] sp : ffff800082b9bd20
> [   10.124202] x29: ffff800082b9bd20 x28: ffff00000000ee05 x27: ffff0000002f21c0
> [   10.134735] x26: ffff000002931820 x25: 61c8864680b583eb x24: ffff0000002f21b8
> [   10.145209] x23: ffff00000026e740 x22: ffff0000002f21e0 x21: ffffffffffffffac
> [   10.155686] x20: ffff000000da5c00 x19: 0000000000000006 x18: 0000000000000000
> [   10.166178] x17: ffff7fffbe0e7000 x16: ffff800080000000 x15: 000039966db1c650
> [   10.176564] x14: 000000000000022c x13: 000000000000022c x12: 0000000000000000
> [   10.186979] x11: 000000000000000a x10: 0000000000000a60 x9 : ffff800082b9bb80
> [   10.197352] x8 : ffff00000026f200 x7 : ffff00003fd90080 x6 : 00000000000022e5
> [   10.207680] x5 : 00000000410fd030 x4 : 0000000000c0000e x3 : ffff7fffbe0e7000
> [   10.218051] x2 : 0000000000000002 x1 : 0000000000000000 x0 : 0000000000000000

Please trim this. No need to dump registers into the commit log. Also 
the module list is usually not so useful. You can prune everything after 
mwifiex.

> [   10.228393] Call trace:
> [   10.233989]  ttyport_receive_buf+0xd8/0xf8
> [   10.241224]  flush_to_ldisc+0xbc/0x1a4
> [   10.248117]  process_scheduled_works+0x16c/0x28c

And these are as well not interesting:

> [   10.255851]  worker_thread+0x16c/0x2e0
> [   10.262673]  kthread+0x11c/0x128
> [   10.268953]  ret_from_fork+0x10/0x20
> [   10.275460] ---[ end trace 0000000000000000 ]---

   ^^^^^^^^^^^^^^
So are not the timestamps.

> Closes: https://lore.kernel.org/all/ZWEIhcUXfutb5SY6@francesco-nb.int.toradex.com/
> Fixes: 689ca16e5232 ("Bluetooth: NXP: Add protocol support for NXP Bluetooth chipsets")
> Signed-off-by: Francesco Dolcini <francesco.dolcini@toradex.com>
> ---
>   drivers/bluetooth/btnxpuart.c | 7 +++----
>   1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/bluetooth/btnxpuart.c b/drivers/bluetooth/btnxpuart.c
> index b7e66b7ac570..951fe3014a3f 100644
> --- a/drivers/bluetooth/btnxpuart.c
> +++ b/drivers/bluetooth/btnxpuart.c
> @@ -1276,11 +1276,10 @@ static int btnxpuart_receive_buf(struct serdev_device *serdev, const u8 *data,
>   	if (IS_ERR(nxpdev->rx_skb)) {
>   		int err = PTR_ERR(nxpdev->rx_skb);
>   		/* Safe to ignore out-of-sync bootloader signatures */
> -		if (is_fw_downloading(nxpdev))
> -			return count;
> -		bt_dev_err(nxpdev->hdev, "Frame reassembly failed (%d)", err);
> +		if (!is_fw_downloading(nxpdev))
> +			bt_dev_err(nxpdev->hdev, "Frame reassembly failed (%d)", err);
>   		nxpdev->rx_skb = NULL;

Is this NULLing not needed in the good case?

> -		return err;
> +		return count;

Should you return 0? I don't know, maybe not, but you should document it 
in the commit log.

thanks,
-- 
js
suse labs

Re: [PATCH v1 1/3] Bluetooth: btnxpuart: fix recv_buf() return value

[Francesco Dolcini]

On Tue, Nov 28, 2023 at 06:23:21AM +0100, Jiri Slaby wrote:
> On 27. 11. 23, 20:14, Francesco Dolcini wrote:
> > From: Francesco Dolcini <francesco.dolcini@toradex.com>
> > 
> > Serdev recv_buf() callback is supposed to return the amount of bytes
> > consumed, therefore an int in between 0 and count.
> > 
> > Do not return negative number in case of issue, just print an error and
> > return count.  This fixes a WARN in ttyport_receive_buf().

...

> >   drivers/bluetooth/btnxpuart.c | 7 +++----
> >   1 file changed, 3 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/bluetooth/btnxpuart.c b/drivers/bluetooth/btnxpuart.c
> > index b7e66b7ac570..951fe3014a3f 100644
> > --- a/drivers/bluetooth/btnxpuart.c
> > +++ b/drivers/bluetooth/btnxpuart.c
> > @@ -1276,11 +1276,10 @@ static int btnxpuart_receive_buf(struct serdev_device *serdev, const u8 *data,
> >   	if (IS_ERR(nxpdev->rx_skb)) {
> >   		int err = PTR_ERR(nxpdev->rx_skb);
> >   		/* Safe to ignore out-of-sync bootloader signatures */
> > -		if (is_fw_downloading(nxpdev))
> > -			return count;
> > -		bt_dev_err(nxpdev->hdev, "Frame reassembly failed (%d)", err);
> > +		if (!is_fw_downloading(nxpdev))
> > +			bt_dev_err(nxpdev->hdev, "Frame reassembly failed (%d)", err);
> >   		nxpdev->rx_skb = NULL;
> 
> Is this NULLing not needed in the good case?
NULLing in the good case would be a bug, in addition to that NULLing is
not needed at all even in the bad case and it will be removed in the
last patch, as a cleanup. Here I just maintained the existing logic.

> > -		return err;
> > +		return count;
> 
> Should you return 0? I don't know, maybe not
My reasoning is that we have some corrupted data, so we should just
use it all and maybe we'll get something valid at a later point, this is
what was already done before this change in the is_fw_downloading()
branch.

In my specific case it makes no difference, it will never recover from
this state.

Any other opinion?

> but you should document it in the commit log.
Ack

Francesco

[PATCH v1 2/3] Bluetooth: btmtkuart: fix recv_buf() return value

[Francesco Dolcini]

From: Francesco Dolcini <francesco.dolcini@toradex.com>

Serdev recv_buf() callback is supposed to return the amount of bytes
consumed, therefore an int in between 0 and count.

Do not return negative number in case of issue, just print an error and
return count. This fixes a WARN in ttyport_receive_buf().

Link: https://lore.kernel.org/all/087be419-ec6b-47ad-851a-5e1e3ea5cfcc@kernel.org/
Fixes: 7237c4c9ec92 ("Bluetooth: mediatek: Add protocol support for MediaTek serial devices")
Signed-off-by: Francesco Dolcini <francesco.dolcini@toradex.com>
---
 drivers/bluetooth/btmtkuart.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/drivers/bluetooth/btmtkuart.c b/drivers/bluetooth/btmtkuart.c
index 935feab815d9..203a000a84e3 100644
--- a/drivers/bluetooth/btmtkuart.c
+++ b/drivers/bluetooth/btmtkuart.c
@@ -336,7 +336,7 @@ mtk_stp_split(struct btmtkuart_dev *bdev, const unsigned char *data, int count,
 	return data;
 }
 
-static int btmtkuart_recv(struct hci_dev *hdev, const u8 *data, size_t count)
+static void btmtkuart_recv(struct hci_dev *hdev, const u8 *data, size_t count)
 {
 	struct btmtkuart_dev *bdev = hci_get_drvdata(hdev);
 	const unsigned char *p_left = data, *p_h4;
@@ -375,25 +375,20 @@ static int btmtkuart_recv(struct hci_dev *hdev, const u8 *data, size_t count)
 			bt_dev_err(bdev->hdev,
 				   "Frame reassembly failed (%d)", err);
 			bdev->rx_skb = NULL;
-			return err;
+			return;
 		}
 
 		sz_left -= sz_h4;
 		p_left += sz_h4;
 	}
-
-	return 0;
 }
 
 static int btmtkuart_receive_buf(struct serdev_device *serdev, const u8 *data,
 				 size_t count)
 {
 	struct btmtkuart_dev *bdev = serdev_device_get_drvdata(serdev);
-	int err;
 
-	err = btmtkuart_recv(bdev->hdev, data, count);
-	if (err < 0)
-		return err;
+	btmtkuart_recv(bdev->hdev, data, count);
 
 	bdev->hdev->stat.byte_rx += count;
 
-- 
2.25.1

[PATCH v1 3/3] Bluetooth: btnxpuart: remove useless assignment

[Francesco Dolcini]

From: Francesco Dolcini <francesco.dolcini@toradex.com>

Remove useless assignment of rx_skb to NULL in case the skb is in error,
this is already done in h4_recv_buf() that is executed a few lines
before.

Signed-off-by: Francesco Dolcini <francesco.dolcini@toradex.com>
---
 drivers/bluetooth/btnxpuart.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/bluetooth/btnxpuart.c b/drivers/bluetooth/btnxpuart.c
index 951fe3014a3f..b7c56be078f8 100644
--- a/drivers/bluetooth/btnxpuart.c
+++ b/drivers/bluetooth/btnxpuart.c
@@ -1278,7 +1278,6 @@ static int btnxpuart_receive_buf(struct serdev_device *serdev, const u8 *data,
 		/* Safe to ignore out-of-sync bootloader signatures */
 		if (!is_fw_downloading(nxpdev))
 			bt_dev_err(nxpdev->hdev, "Frame reassembly failed (%d)", err);
-		nxpdev->rx_skb = NULL;
 		return count;
 	}
 	if (!is_fw_downloading(nxpdev))
-- 
2.25.1

Re: [PATCH v1 3/3] Bluetooth: btnxpuart: remove useless assignment

[Jiri Slaby]

On 27. 11. 23, 20:14, Francesco Dolcini wrote:
> From: Francesco Dolcini <francesco.dolcini@toradex.com>
> 
> Remove useless assignment of rx_skb to NULL in case the skb is in error,
> this is already done in h4_recv_buf() that is executed a few lines
> before.

In case of error, nxpdev->rx_skb contains the error, not NULL, right?

> Signed-off-by: Francesco Dolcini <francesco.dolcini@toradex.com>
> ---
>   drivers/bluetooth/btnxpuart.c | 1 -
>   1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/bluetooth/btnxpuart.c b/drivers/bluetooth/btnxpuart.c
> index 951fe3014a3f..b7c56be078f8 100644
> --- a/drivers/bluetooth/btnxpuart.c
> +++ b/drivers/bluetooth/btnxpuart.c
> @@ -1278,7 +1278,6 @@ static int btnxpuart_receive_buf(struct serdev_device *serdev, const u8 *data,
>   		/* Safe to ignore out-of-sync bootloader signatures */
>   		if (!is_fw_downloading(nxpdev))
>   			bt_dev_err(nxpdev->hdev, "Frame reassembly failed (%d)", err);
> -		nxpdev->rx_skb = NULL;

thanks,
-- 
js
suse labs

Re: [PATCH v1 3/3] Bluetooth: btnxpuart: remove useless assignment

[Francesco Dolcini]

On Tue, Nov 28, 2023 at 06:26:56AM +0100, Jiri Slaby wrote:
> On 27. 11. 23, 20:14, Francesco Dolcini wrote:
> > From: Francesco Dolcini <francesco.dolcini@toradex.com>
> > 
> > Remove useless assignment of rx_skb to NULL in case the skb is in error,
> > this is already done in h4_recv_buf() that is executed a few lines
> > before.
> 
> In case of error, nxpdev->rx_skb contains the error, not NULL, right?
correct.

The point is that we have

  if (IS_ERR(skb))
    skb = NULL;

at the beginning of h4_recv_buf(), that is called a few lines before,
making the assignment in btnxpuart.c redundant.

Francesco

Re: [PATCH v1 0/3] Bluetooth: fix recv_buf() return value

[Francesco Dolcini]

Hello Jiri,

On Mon, Nov 27, 2023 at 08:14:05PM +0100, Francesco Dolcini wrote:
> From: Francesco Dolcini <francesco.dolcini@toradex.com>
> 
> Serdev recv_buf() callback is supposed to return the amount of bytes
> consumed, therefore an int in between 0 and count.

I have also a patch ready to convert the return value of serdev
recv_buf() from int to size_t.

I would be inclined to wait for this series to go though first, given
that these are fixes, while the change from int to size_t is just a
cleanup to prevent future mistakes. Do you agree of would you do it
differently?

Francesco

Re: [PATCH v1 0/3] Bluetooth: fix recv_buf() return value

[Jiri Slaby]

Hi,

On 27. 11. 23, 20:23, Francesco Dolcini wrote:
> On Mon, Nov 27, 2023 at 08:14:05PM +0100, Francesco Dolcini wrote:
>> From: Francesco Dolcini <francesco.dolcini@toradex.com>
>>
>> Serdev recv_buf() callback is supposed to return the amount of bytes
>> consumed, therefore an int in between 0 and count.
> 
> I have also a patch ready to convert the return value of serdev
> recv_buf() from int to size_t.
> 
> I would be inclined to wait for this series to go though first, given
> that these are fixes, while the change from int to size_t is just a
> cleanup to prevent future mistakes. Do you agree of would you do it
> differently?

Fine by me either way. You can include it in this series at the end. 
Fixes can be picked up by stable too, the rest would go to mainline only.

thanks,
-- 
js
suse labs