[PATCH] mbim driver: call the release function in case MBIM OPEN fails

[PATCH] mbim driver: call the release function in case MBIM OPEN fails

[Giacinto Cifelli]

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

---
 drivers/mbimmodem/mbim.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/mbimmodem/mbim.c b/drivers/mbimmodem/mbim.c
index 9fcf44b..f4132d0 100644
--- a/drivers/mbimmodem/mbim.c
+++ b/drivers/mbimmodem/mbim.c
@@ -781,6 +781,9 @@ static bool open_read_handler(struct l_io *io, void *user_data)
 	/* Grab OPEN_DONE Status field */
 	if (l_get_le32(buf) != 0) {
 		close(fd);
+		if (device->disconnect_handler)
+			device->disconnect_handler(device->ready_data);
+		device->is_ready = false;
 		return false;
 	}
 
-- 
1.9.1

Re: [PATCH] mbim driver: call the release function in case MBIM OPEN fails

[Denis Kenzior]

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

Hi Giacinto,

An actual description of what is happening would be helpful

On 09/19/2018 11:47 PM, Giacinto Cifelli wrote:
> ---
>   drivers/mbimmodem/mbim.c | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/mbimmodem/mbim.c b/drivers/mbimmodem/mbim.c
> index 9fcf44b..f4132d0 100644
> --- a/drivers/mbimmodem/mbim.c
> +++ b/drivers/mbimmodem/mbim.c
> @@ -781,6 +781,9 @@ static bool open_read_handler(struct l_io *io, void *user_data)
>   	/* Grab OPEN_DONE Status field */
>   	if (l_get_le32(buf) != 0) {
>   		close(fd);
> +		if (device->disconnect_handler)
> +			device->disconnect_handler(device->ready_data);
> +		device->is_ready = false;

I don't get this part.  So device->is_ready = false assignment is 
pointless.  It is already false.

Also, aren't we going to call device->disconnect_handler anyway by 
virtue of closing the fd?  l_io_set_disconnect_handler which we set in 
mbim_device_new should already be taking care of this.

Regards,
-Denis

Re: [PATCH] mbim driver: call the release function in case MBIM OPEN fails

[Giacinto Cifelli]

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

Hi Denis,

On Thu, Sep 20, 2018 at 5:48 PM Denis Kenzior <denkenz@gmail.com> wrote:

> Hi Giacinto,
>
> An actual description of what is happening would be helpful
>
> On 09/19/2018 11:47 PM, Giacinto Cifelli wrote:
> > ---
> >   drivers/mbimmodem/mbim.c | 3 +++
> >   1 file changed, 3 insertions(+)
> >
> > diff --git a/drivers/mbimmodem/mbim.c b/drivers/mbimmodem/mbim.c
> > index 9fcf44b..f4132d0 100644
> > --- a/drivers/mbimmodem/mbim.c
> > +++ b/drivers/mbimmodem/mbim.c
> > @@ -781,6 +781,9 @@ static bool open_read_handler(struct l_io *io, void
> *user_data)
> >       /* Grab OPEN_DONE Status field */
> >       if (l_get_le32(buf) != 0) {
> >               close(fd);
> > +             if (device->disconnect_handler)
> > +                     device->disconnect_handler(device->ready_data);
> > +             device->is_ready = false


>
I don't get this part.  So device->is_ready = false assignment is
> pointless.  It is already false.
>
> Also, aren't we going to call device->disconnect_handler anyway by
> virtue of closing the fd?  l_io_set_disconnect_handler which we set in
> mbim_device_new should already be taking care of this.
>
>
I will check about the is_ready false, but definitely the
disconnect_handler is not called, I can tell you,
because on a special condition I have received a OPEN DONE with failure,
and the driver remained hanging.
However, after calling it, there is a second call to disconnect_handler,
but not if it is not done here.
Maybe the code should be verified somewhere else? Any other suggestions?

Regards,
> -Denis
>

Regards,
Giacinto

[-- Attachment #2: attachment.html --]
[-- Type: text/html, Size: 2480 bytes --]

Re: [PATCH] mbim driver: call the release function in case MBIM OPEN fails

[Denis Kenzior]

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

Hi Giacinto,

> I will check about the is_ready false, but definitely the 
> disconnect_handler is not called, I can tell you,
> because on a special condition I have received a OPEN DONE with failure, 
> and the driver remained hanging.
> However, after calling it, there is a second call to disconnect_handler, 
> but not if it is not done here.
> Maybe the code should be verified somewhere else? Any other suggestions?

Okay, I played with this a bit and indeed there was an issue.  It should 
be fixed in ell upstream now.

See commit 0294175cff974ae9aae93e3609c5f6502db410ee.

Regards,
-Denis

Re: [PATCH] mbim driver: call the release function in case MBIM OPEN fails

[Giacinto Cifelli]

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

Hi Denis,

On Fri, Sep 21, 2018 at 12:01 AM Denis Kenzior <denkenz@gmail.com> wrote:

> Hi Giacinto,
>
> > I will check about the is_ready false, but definitely the
> > disconnect_handler is not called, I can tell you,
> > because on a special condition I have received a OPEN DONE with failure,
> > and the driver remained hanging.
> > However, after calling it, there is a second call to disconnect_handler,
> > but not if it is not done here.
> > Maybe the code should be verified somewhere else? Any other suggestions?
>
> Okay, I played with this a bit and indeed there was an issue.  It should
> be fixed in ell upstream now.
>
> See commit 0294175cff974ae9aae93e3609c5f6502db410ee.
>

It works with this ell patch, and the disconnect_handler is called once.
Thank you very much.


>
> Regards,
> -Denis
>

Regards,
Giacinto

[-- Attachment #2: attachment.html --]
[-- Type: text/html, Size: 1339 bytes --]