| Message ID | 20250128210354.73732-1-pchelkin@ispras.ru (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [BlueZ,1/2] audio: actually try to enable MTU auto-tuning | expand |
| Context | Check | Description |
|---|---|---|
| tedd_an/pre-ci_am | success | Success |
| tedd_an/BuildEll | success | Build ELL PASS |
| tedd_an/BluezMake | success | Bluez Make PASS |
| tedd_an/MakeCheck | success | Bluez Make Check PASS |
| tedd_an/MakeDistcheck | success | Make Distcheck PASS |
| tedd_an/CheckValgrind | success | Check Valgrind PASS |
| tedd_an/CheckSmatch | success | CheckSparse PASS |
| tedd_an/bluezmakeextell | success | Make External ELL PASS |
| tedd_an/ScanBuild | success | Scan Build PASS |
Hi Fedor, On Tue, Jan 28, 2025 at 4:04 PM Fedor Pchelkin <pchelkin@ispras.ru> wrote: > > A "0" for the input MTU passed to the underlying socket is supposed to > indicate that its value should be determined by the L2CAP layer. > However, the current code treats a zero imtu just as if there is > nothing to change. > > Introduce an additional flag to indicate that the zero imtu is > explicitly requested by the caller for the purpose of auto-tuning. > Otherwise, the similar behavior remains. > > Found by Linux Verification Center (linuxtesting.org). > > Fixes: ae5be371a9f5 ("avdtp: Enable MTU auto tunning") > --- > btio/btio.c | 23 +++++++++++++---------- > 1 file changed, 13 insertions(+), 10 deletions(-) > > diff --git a/btio/btio.c b/btio/btio.c > index 2d277e409..74a4003b6 100644 > --- a/btio/btio.c > +++ b/btio/btio.c > @@ -66,6 +66,7 @@ struct set_opts { > uint16_t imtu; > uint16_t omtu; > int central; > + uint8_t auto_mtu; > uint8_t mode; > int flushable; > uint32_t priority; > @@ -610,7 +611,7 @@ static uint8_t mode_l2mode(uint8_t mode) > } > > static gboolean set_l2opts(int sock, uint16_t imtu, uint16_t omtu, > - uint8_t mode, GError **err) > + uint8_t auto_mtu, uint8_t mode, GError **err) > { > struct l2cap_options l2o; > socklen_t len; > @@ -622,7 +623,7 @@ static gboolean set_l2opts(int sock, uint16_t imtu, uint16_t omtu, > return FALSE; > } > > - if (imtu) > + if (imtu || auto_mtu) > l2o.imtu = imtu; We might need to do some more special handling for auto_mtu, so in case it fail we retry with the default values instead. > if (omtu) > l2o.omtu = omtu; > @@ -666,17 +667,17 @@ static gboolean set_le_mode(int sock, uint8_t mode, GError **err) > } > > static gboolean l2cap_set(int sock, uint8_t src_type, int sec_level, > - uint16_t imtu, uint16_t omtu, uint8_t mode, > - int central, int flushable, uint32_t priority, > - GError **err) > + uint16_t imtu, uint16_t omtu, uint8_t auto_mtu, > + uint8_t mode, int central, int flushable, > + uint32_t priority, GError **err) > { > - if (imtu || omtu || mode) { > + if (imtu || omtu || auto_mtu || mode) { > gboolean ret = FALSE; > > if (src_type == BDADDR_BREDR) > - ret = set_l2opts(sock, imtu, omtu, mode, err); > + ret = set_l2opts(sock, imtu, omtu, auto_mtu, mode, err); Perhaps here we do: if (ret && auto_mtu) ret = set_l2opts(sock, imtu, omtu, false, mode, err); Thoughts? > else { > - if (imtu) > + if (imtu || auto_mtu) > ret = set_le_imtu(sock, imtu, err); > > if (ret && mode) > @@ -986,6 +987,8 @@ static gboolean parse_set_opts(struct set_opts *opts, GError **err, > opts->imtu = va_arg(args, int); > if (!opts->mtu) > opts->mtu = opts->imtu; > + if (!opts->imtu) > + opts->auto_mtu = 1; > break; > case BT_IO_OPT_CENTRAL: > opts->central = va_arg(args, gboolean); > @@ -1890,7 +1893,7 @@ gboolean bt_io_set(GIOChannel *io, GError **err, BtIOOption opt1, ...) > switch (type) { > case BT_IO_L2CAP: > return l2cap_set(sock, opts.src_type, opts.sec_level, opts.imtu, > - opts.omtu, opts.mode, opts.central, > + opts.omtu, opts.auto_mtu, opts.mode, opts.central, > opts.flushable, opts.priority, err); > case BT_IO_RFCOMM: > return rfcomm_set(sock, opts.sec_level, opts.central, err); > @@ -1941,7 +1944,7 @@ static GIOChannel *create_io(gboolean server, struct set_opts *opts, > server ? opts->psm : 0, opts->cid, err) < 0) > goto failed; > if (!l2cap_set(sock, opts->src_type, opts->sec_level, > - opts->imtu, opts->omtu, opts->mode, > + opts->imtu, opts->omtu, opts->auto_mtu, opts->mode, > opts->central, opts->flushable, opts->priority, > err)) > goto failed; > -- > 2.39.5 >
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=928933 ---Test result--- Test Summary: CheckPatch PENDING 0.25 seconds GitLint PENDING 0.21 seconds BuildEll PASS 21.19 seconds BluezMake PASS 1617.71 seconds MakeCheck PASS 12.80 seconds MakeDistcheck PASS 165.05 seconds CheckValgrind PASS 221.45 seconds CheckSmatch PASS 279.76 seconds bluezmakeextell PASS 100.71 seconds IncrementalBuild PENDING 0.27 seconds ScanBuild PASS 899.96 seconds Details ############################## Test: CheckPatch - PENDING Desc: Run checkpatch.pl script Output: ############################## Test: GitLint - PENDING Desc: Run gitlint Output: ############################## Test: IncrementalBuild - PENDING Desc: Incremental build with the patches in the series Output: --- Regards, Linux Bluetooth
On Tue, 28. Jan 16:59, Luiz Augusto von Dentz wrote: > > @@ -622,7 +623,7 @@ static gboolean set_l2opts(int sock, uint16_t imtu, uint16_t omtu, > > return FALSE; > > } > > > > - if (imtu) > > + if (imtu || auto_mtu) > > l2o.imtu = imtu; > > We might need to do some more special handling for auto_mtu, so in > case it fail we retry with the default values instead. Yep, a good point. And I see now it _might_ fail here for the kernels without the corresponding patch. > > static gboolean l2cap_set(int sock, uint8_t src_type, int sec_level, > > - uint16_t imtu, uint16_t omtu, uint8_t mode, > > - int central, int flushable, uint32_t priority, > > - GError **err) > > + uint16_t imtu, uint16_t omtu, uint8_t auto_mtu, > > + uint8_t mode, int central, int flushable, > > + uint32_t priority, GError **err) > > { > > - if (imtu || omtu || mode) { > > + if (imtu || omtu || auto_mtu || mode) { > > gboolean ret = FALSE; > > > > if (src_type == BDADDR_BREDR) > > - ret = set_l2opts(sock, imtu, omtu, mode, err); > > + ret = set_l2opts(sock, imtu, omtu, auto_mtu, mode, err); > > Perhaps here we do: > > if (ret && auto_mtu) > ret = set_l2opts(sock, imtu, omtu, false, mode, err); > > Thoughts? Agreed, trying the original default behavior will work with existing kernels. I'll respin the series. > > > else { > > - if (imtu) > > + if (imtu || auto_mtu) > > ret = set_le_imtu(sock, imtu, err); Huh, the BT_RCVMTU case may also fail for L2CAP_MODE_EXT_FLOWCTL (that's not something I'm prepared to reproduce though). l2cap_chan_reconfigure() will reject a zero imtu. Worth adding some another kernel patch or ECRED should not actually support MTU auto-tuning?
diff --git a/btio/btio.c b/btio/btio.c index 2d277e409..74a4003b6 100644 --- a/btio/btio.c +++ b/btio/btio.c @@ -66,6 +66,7 @@ struct set_opts { uint16_t imtu; uint16_t omtu; int central; + uint8_t auto_mtu; uint8_t mode; int flushable; uint32_t priority; @@ -610,7 +611,7 @@ static uint8_t mode_l2mode(uint8_t mode) } static gboolean set_l2opts(int sock, uint16_t imtu, uint16_t omtu, - uint8_t mode, GError **err) + uint8_t auto_mtu, uint8_t mode, GError **err) { struct l2cap_options l2o; socklen_t len; @@ -622,7 +623,7 @@ static gboolean set_l2opts(int sock, uint16_t imtu, uint16_t omtu, return FALSE; } - if (imtu) + if (imtu || auto_mtu) l2o.imtu = imtu; if (omtu) l2o.omtu = omtu; @@ -666,17 +667,17 @@ static gboolean set_le_mode(int sock, uint8_t mode, GError **err) } static gboolean l2cap_set(int sock, uint8_t src_type, int sec_level, - uint16_t imtu, uint16_t omtu, uint8_t mode, - int central, int flushable, uint32_t priority, - GError **err) + uint16_t imtu, uint16_t omtu, uint8_t auto_mtu, + uint8_t mode, int central, int flushable, + uint32_t priority, GError **err) { - if (imtu || omtu || mode) { + if (imtu || omtu || auto_mtu || mode) { gboolean ret = FALSE; if (src_type == BDADDR_BREDR) - ret = set_l2opts(sock, imtu, omtu, mode, err); + ret = set_l2opts(sock, imtu, omtu, auto_mtu, mode, err); else { - if (imtu) + if (imtu || auto_mtu) ret = set_le_imtu(sock, imtu, err); if (ret && mode) @@ -986,6 +987,8 @@ static gboolean parse_set_opts(struct set_opts *opts, GError **err, opts->imtu = va_arg(args, int); if (!opts->mtu) opts->mtu = opts->imtu; + if (!opts->imtu) + opts->auto_mtu = 1; break; case BT_IO_OPT_CENTRAL: opts->central = va_arg(args, gboolean); @@ -1890,7 +1893,7 @@ gboolean bt_io_set(GIOChannel *io, GError **err, BtIOOption opt1, ...) switch (type) { case BT_IO_L2CAP: return l2cap_set(sock, opts.src_type, opts.sec_level, opts.imtu, - opts.omtu, opts.mode, opts.central, + opts.omtu, opts.auto_mtu, opts.mode, opts.central, opts.flushable, opts.priority, err); case BT_IO_RFCOMM: return rfcomm_set(sock, opts.sec_level, opts.central, err); @@ -1941,7 +1944,7 @@ static GIOChannel *create_io(gboolean server, struct set_opts *opts, server ? opts->psm : 0, opts->cid, err) < 0) goto failed; if (!l2cap_set(sock, opts->src_type, opts->sec_level, - opts->imtu, opts->omtu, opts->mode, + opts->imtu, opts->omtu, opts->auto_mtu, opts->mode, opts->central, opts->flushable, opts->priority, err)) goto failed;
A "0" for the input MTU passed to the underlying socket is supposed to indicate that its value should be determined by the L2CAP layer. However, the current code treats a zero imtu just as if there is nothing to change. Introduce an additional flag to indicate that the zero imtu is explicitly requested by the caller for the purpose of auto-tuning. Otherwise, the similar behavior remains. Found by Linux Verification Center (linuxtesting.org). Fixes: ae5be371a9f5 ("avdtp: Enable MTU auto tunning") --- btio/btio.c | 23 +++++++++++++---------- 1 file changed, 13 insertions(+), 10 deletions(-)