[PATCH] bearer-mbim: avoid calling mbim_message_unref on NULL MbimMessage

2017-07-26 Thread Ben Chan 
---
 src/mm-bearer-mbim.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/src/mm-bearer-mbim.c b/src/mm-bearer-mbim.c
index 439de812..63bb0579 100644
--- a/src/mm-bearer-mbim.c
+++ b/src/mm-bearer-mbim.c
@@ -173,7 +173,9 @@ packet_statistics_query_ready (MbimDevice *device,
 g_task_return_error (task, error);
 
 g_object_unref (task);
-mbim_message_unref (response);
+
+if (response)
+mbim_message_unref (response);
 }
 
 static void
-- 
2.14.0.rc0.400.g1c36432dff-goog

___
ModemManager-devel mailing list
ModemManager-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/modemmanager-devel

Re: [PATCH] broadband-bearer: run init sequence after flashing in disconnection

2017-07-26 Thread Aleksander Morgado 
>
> Incidentally, a similar thing goes for RTS/CTS, these aren't always
> connected up, but the flow control setup logic currently says "if the
> host supports RTS/CTS and the modem supports RTS/CTS, then turn it on",
> which might not be the right thing to do since they might not actually
> be connected to each other.  Maybe a udev set of rules like:
>
> ENV{ID_MM_TTY_DTR_BROKEN}="1"
>
> ENV{ID_MM_TTY_RTSCTS_BROKEN}="1"
>
> is the right thing to do?
>

I'm assuming you're thinking in users providing these udev tags for
custom setups, right?

>> Or if the modem doesn't, we could send the breaks.  If the modem
>> doesn't actually drop to command mode () then we could also send
>> breaks.
>
> The whole break thing might be a red herring, since although some modems
> will respond to this, I don't think its widespread any more (a quick
> google shows that it would need to be configured AT\K.
>
> I think probably the debug comment should be changed to "attempting to
> hang up the modem by dropping serial port DTR signal" or similar, since
> "flashing the serial port" is a bit of a strange usage (and I'd guess
> that if people get anything from that description it might be a serial
> break, especially since setting the baud rate to zero is how you do a
> serial break on some platforms).
>
> The two reasonable/reliable ways of ending a call are DTR on->off
> transition (if enabled via AT - enter command mode and also end
> current data call), and also sending the escape sequence followed by ATH
> "hang up".  I think.
>

Out of curiosity; are you testing the connection/disconnection of the
TTYs in MM with a running pppd when it gets connected? Or just doing
--simple-connect and then --simple-disconnect for example via mmcli?
When pppd is in place, most of the modems get disconnected right away
when the daemon exits, and our disconnection logic afterwards doesn't
really do much (is pppd maybe doing DTR on->off itself on
disconnection?). That's what I recall anyway, it really is a long time
ago since I played with PPP based modems.

The generic broadband bearer, the one that is used to setup PPP over
TTY, should really be an object that implements as many procedures as
possible to successfully disconnect a call; whether plugins then end
up disabling some of those steps is something that can be done later
on. So if we add additional disconnect methods, I'm personally fine,
assuming they don't interfere with other modems that use other methods
successfully.

-- 
Aleksander
https://aleksander.es
___
ModemManager-devel mailing list
ModemManager-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/modemmanager-devel

Re: ATZ in enabling_modem_init() step

2017-07-26 Thread Aleksander Morgado 
On Wed, Jul 26, 2017 at 4:32 PM, Tim Small  wrote:
> On 25/07/17 12:47, Aleksander Morgado wrote:
>> A quick solution would be to subclass the enabling_modem_init() step
>> and after running the parent method, run a Telit specific setup to
>> check #CSS? and if disabled due to the ATZ, re-set it, would that
>> work?
>
> I wonder if issuing ATZ is the right thing to do here at all?
>
> It's trying to reset the modem to a well-known state, but ATZ doesn't do
> that - it loads the default user defined profile - which could be
> anything that the user or another piece of software has saved with AT
>

I guess it all depends on the modem in use. Plus, yeah, users may
configure the modem in some way that may then not work correctly with
ModemManager's logic; we have to live with that I'd say. Users may
even configure frequency bands that are not in use in the location
where the modem is trying to run, and MM wouldn't be able to do
anything either. If the user does custom configurations, the user
should deal with problems that may arise from those. MM should only
try to take care of the configurations initiated via APIs provided by
MM; any other use case should be treated out of scope, IMO. That said,
it doesn't mean that users shouldn't do their own configs; but if they
do, maybe it's a good idea to have them done directly via MM, so that
MM can keep the state of what's being done. There are tons of modem
configs, including TTY configs, that we blindly ignore in MM.

> Running AT once during/after probe might be better to get the modem
> to a known state.
>
> Even more complicated, having just checked the Telit modem CMUX user
> guide - when in CMUX mode both the ATZ and AT commands are ignored, so
> it would be necessary to issue AT before entering CMUX mode, but
> neither would be an option after that.
>

Are you testing MM with a TTY exposing multiple virtual channels via
CMUX already? How's your setup?

-- 
Aleksander
https://aleksander.es
___
ModemManager-devel mailing list
ModemManager-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/modemmanager-devel

Re: Modem index increment

2017-07-26 Thread Aleksander Morgado 
On Wed, Jul 26, 2017 at 4:57 PM, José  wrote:
> I have noticed that when unplugging and plugging a modem, ModemManager
> increases its index by one. This can be annoying when you are trying
> to script something.
>
> Would it make sense to assign the same index to the same modem? Maybe
> something like using the  "equipment id" field to detect a plugged
> modem is not new and use the previous index, instead of increasing it.
>
> Has this been consider before? Are there any arguments for or against it?

You can apply a "unique id" to each modem via udev tags (e.g. based on
the usb sysfs path), and then use that to refer to the modem in mmcli
calls, see:
https://sigquit.wordpress.com/2016/10/06/naming-devices-in-modemmanager/

Would that help your use case?

-- 
Aleksander
https://aleksander.es
___
ModemManager-devel mailing list
ModemManager-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/modemmanager-devel

Modem index increment

2017-07-26 Thread José 
I have noticed that when unplugging and plugging a modem, ModemManager
increases its index by one. This can be annoying when you are trying
to script something.

Would it make sense to assign the same index to the same modem? Maybe
something like using the  "equipment id" field to detect a plugged
modem is not new and use the previous index, instead of increasing it.

Has this been consider before? Are there any arguments for or against it?
___
ModemManager-devel mailing list
ModemManager-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/modemmanager-devel

ATZ in enabling_modem_init() step

2017-07-26 Thread Tim Small 
On 25/07/17 12:47, Aleksander Morgado wrote:
> A quick solution would be to subclass the enabling_modem_init() step
> and after running the parent method, run a Telit specific setup to
> check #CSS? and if disabled due to the ATZ, re-set it, would that
> work?

I wonder if issuing ATZ is the right thing to do here at all?

It's trying to reset the modem to a well-known state, but ATZ doesn't do
that - it loads the default user defined profile - which could be
anything that the user or another piece of software has saved with AT

Running AT once during/after probe might be better to get the modem
to a known state.

Even more complicated, having just checked the Telit modem CMUX user
guide - when in CMUX mode both the ATZ and AT commands are ignored, so
it would be necessary to issue AT before entering CMUX mode, but
neither would be an option after that.

Tim.

-- 
South East Open Source Solutions Limited
Registered in England and Wales with company number 06134732.
Registered Office: 2 Powell Gardens, Redhill, Surrey, RH1 1TQ
VAT number: 900 6633 53  http://seoss.co.uk/ +44-(0)1273-808309
___
ModemManager-devel mailing list
ModemManager-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/modemmanager-devel

Re: [PATCH] sim hot swap: improved error management

2017-07-26 Thread Tim Small 
On 25/07/17 13:05, Carlo Lobrano wrote:
> I can't reproduce this issue. I see the ATZ command, but I still receive
> each QSS notification.

OK.  Do you have separate data and command ports?  Is so, then perhaps
the reason which you can't reproduce the issue is that the ATZ command
only acts locally on the AT command interpreter for the port which its
issued on...

If you enable sim hot swap notification on an aux/command port, and the
ATZ is issued to the data port, then sim hot swap will still be enabled
on the aux port.

If you only have a single port (which is the case with the modem I'm
testing with - unless I use CMUX mode), and hot swap notification has
been enabled, then issuing ATZ on that port will turn off hot swap
notification.

Cheers,

Tim.

-- 
South East Open Source Solutions Limited
Registered in England and Wales with company number 06134732.
Registered Office: 2 Powell Gardens, Redhill, Surrey, RH1 1TQ
VAT number: 900 6633 53  http://seoss.co.uk/ +44-(0)1273-808309
___
ModemManager-devel mailing list
ModemManager-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/modemmanager-devel

Re: [PATCH] broadband-bearer: run init sequence after flashing in disconnection

2017-07-26 Thread Tim Small 
On 25/07/17 16:46, Dan Williams wrote:
> There's a couple more things we could do with the disconnect code too.
> 
> As you suggest, we could set AT by default and if the
> modem allows that, hope that dropping DTR does the right thing.

A potential problem here is badly implemented hardware where the DTR
line is disconnected and left floating, you might get spurious random
transitions on that line.

An alternative (which should at least be an option I think and will work
even when the DTR line isn't in use) is to use the +++ escape sequence:

1. 
2. send "+++"
3. 

The modem then re-enters command mode.  Any ongoing data call can then
be ended with:

ATH

or returned to using:

ATO

incidentally, all queued unsolicited status updates will be sent to the
host as soon as the command mode is entered.  The guard interval can be
set with ATS12=xxx - where xxx is the time interval in 50ths of a
second.  The usual escape character is '+', but this can be set/changed
with ATS2=xxx, where the number is an ASCII character.

Incidentally, a similar thing goes for RTS/CTS, these aren't always
connected up, but the flow control setup logic currently says "if the
host supports RTS/CTS and the modem supports RTS/CTS, then turn it on",
which might not be the right thing to do since they might not actually
be connected to each other.  Maybe a udev set of rules like:

ENV{ID_MM_TTY_DTR_BROKEN}="1"

ENV{ID_MM_TTY_RTSCTS_BROKEN}="1"

is the right thing to do?

> Or if the modem doesn't, we could send the breaks.  If the modem
> doesn't actually drop to command mode () then we could also send
> breaks.

The whole break thing might be a red herring, since although some modems
will respond to this, I don't think its widespread any more (a quick
google shows that it would need to be configured AT\K.

I think probably the debug comment should be changed to "attempting to
hang up the modem by dropping serial port DTR signal" or similar, since
"flashing the serial port" is a bit of a strange usage (and I'd guess
that if people get anything from that description it might be a serial
break, especially since setting the baud rate to zero is how you do a
serial break on some platforms).

The two reasonable/reliable ways of ending a call are DTR on->off
transition (if enabled via AT - enter command mode and also end
current data call), and also sending the escape sequence followed by ATH
"hang up".  I think.

Tim.

-- 
South East Open Source Solutions Limited
Registered in England and Wales with company number 06134732.
Registered Office: 2 Powell Gardens, Redhill, Surrey, RH1 1TQ
VAT number: 900 6633 53  http://seoss.co.uk/ +44-(0)1273-808309
___
ModemManager-devel mailing list
ModemManager-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/modemmanager-devel

Re: [PATCH] telit-plugin: ignore QSS when SIM-ME interface is locked

2017-07-26 Thread Carlo Lobrano 
That's right, thank you

On Wed, 26 Jul 2017 at 12:21 Aleksander Morgado 
wrote:

> On Wed, Jul 26, 2017 at 10:36 AM, Carlo Lobrano 
> wrote:
> >>> +csim_unlock_complete (self->priv->csim_lock_task);
> >> Reset the csim_lock_task pointer here to NULL, please.
> >
> > Can I do this inside csim_unlock_complete or there is a reason to do it
> > outside this function?
> >
>
> If you do it inside the function, you should be passing the address of
> the GTask pointer, i.e.:
>   csim_unlock_complete (GTask **task);
> So that you can do *task = NULL; inside the function. That's fine.
>
> Although, maybe, given that the GTask is inside the private structure
> of the modem object, it may make more sense if you pass the modem
> object directly, and access the private info within the function,
> something like:
>
>   static void
>   pending_csim_unlock_complete (MMBroadbandModemTelit *self)
>   {
>   // whatever
>   g_task_return_something();
>   g_clear_object (>priv->csim_lock_task);
>   }
>
>
>
>
>
> --
> Aleksander
> https://aleksander.es
>
___
ModemManager-devel mailing list
ModemManager-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/modemmanager-devel

Re: [PATCH] telit-plugin: ignore QSS when SIM-ME interface is locked

2017-07-26 Thread Carlo Lobrano 
>> +csim_unlock_complete (self->priv->csim_lock_task);
> Reset the csim_lock_task pointer here to NULL, please.

Can I do this inside csim_unlock_complete or there is a reason to do it
outside this function?

On Tue, 25 Jul 2017 at 14:28 Carlo Lobrano  wrote:

> > Not sure you need to receive the GTask here from the timeout
> > user_data, as you already have it in the private info. Instead, you
> > could pass a reference to the MMBroadbandModemTelit object as
> > user_data and receive "self" here.
>
> Totally agree. I was unsure how to manage the two references and I didn't
> think I actually don't need it.
>
>
>
> On Tue, 25 Jul 2017 at 14:10 Aleksander Morgado 
> wrote:
>
>> Hey,
>>
>> On Mon, Jul 24, 2017 at 6:23 PM, Carlo Lobrano 
>> wrote:
>> > With some modems, the lock/unlock of the SIM-ME interface with +CSIM=1/0
>> > command is followed by #QSS unsolicited messages. With the current
>> > implementation, this messages are mistaken for SIM swap events and so
>> the
>> > modem is first dropped and then re-probed.
>> >
>> > With this patch, the plugin takes into account the SIM-ME lock state
>> when
>> > parsing #QSS unsolicited, so that the QSS handler can correctly
>> > elaborate the messages that are not related to SIM swap events.
>> > ---
>> >  plugins/telit/mm-broadband-modem-telit.c | 90
>> 
>> >  plugins/telit/mm-modem-helpers-telit.h   |  9 
>> >  2 files changed, 90 insertions(+), 9 deletions(-)
>> >
>> > diff --git a/plugins/telit/mm-broadband-modem-telit.c
>> b/plugins/telit/mm-broadband-modem-telit.c
>> > index e7650c0..3facc3e 100644
>> > --- a/plugins/telit/mm-broadband-modem-telit.c
>> > +++ b/plugins/telit/mm-broadband-modem-telit.c
>> > @@ -44,6 +44,8 @@ G_DEFINE_TYPE_EXTENDED (MMBroadbandModemTelit,
>> mm_broadband_modem_telit, MM_TYPE
>> >  G_IMPLEMENT_INTERFACE (MM_TYPE_IFACE_MODEM,
>> iface_modem_init)
>> >  G_IMPLEMENT_INTERFACE
>> (MM_TYPE_IFACE_MODEM_3GPP, iface_modem_3gpp_init));
>> >
>> > +#define CSIM_UNLOCK_MAX_TIMEOUT 3
>> > +
>> >  typedef enum {
>> >  FEATURE_SUPPORT_UNKNOWN,
>> >  FEATURE_NOT_SUPPORTED,
>> > @@ -53,6 +55,9 @@ typedef enum {
>> >  struct _MMBroadbandModemTelitPrivate {
>> >  FeatureSupport csim_lock_support;
>> >  MMTelitQssStatus qss_status;
>> > +MMTelitCsimLockState csim_lock_state;
>> > +GTask *csim_lock_task;
>> > +guint csim_lock_timeout_id;
>> >  };
>> >
>> >
>> /*/
>> > @@ -107,6 +112,7 @@ typedef struct {
>> >  } QssSetupContext;
>> >
>> >  static void qss_setup_step (GTask *task);
>> > +static void csim_unlock_complete (GTask *task);
>> >
>> >  static void
>> >  telit_qss_unsolicited_handler (MMPortSerialAt *port,
>> > @@ -122,14 +128,36 @@ telit_qss_unsolicited_handler (MMPortSerialAt
>> *port,
>> >  prev_qss_status = self->priv->qss_status;
>> >  self->priv->qss_status = cur_qss_status;
>> >
>> > +if (self->priv->csim_lock_state >= CSIM_LOCK_STATE_LOCK_REQUESTED)
>> {
>> > +
>> > +if (prev_qss_status > QSS_STATUS_SIM_REMOVED && cur_qss_status
>> == QSS_STATUS_SIM_REMOVED) {
>> > +mm_dbg ("QSS handler: #QSS=0 after +CSIM=1 -> CSIM
>> locked!");
>> > +self->priv->csim_lock_state = CSIM_LOCK_STATE_LOCKED;
>> > +}
>> > +
>> > +if (prev_qss_status == QSS_STATUS_SIM_REMOVED &&
>> cur_qss_status != QSS_STATUS_SIM_REMOVED) {
>> > +mm_dbg ("QSS handler: #QSS>=1 after +CSIM=0 -> CSIM
>> unlocked!");
>> > +self->priv->csim_lock_state = CSIM_LOCK_STATE_UNLOCKED;
>> > +
>> > +if (self->priv->csim_lock_timeout_id) {
>> > +g_source_remove (self->priv->csim_lock_timeout_id);
>> > +self->priv->csim_lock_timeout_id = 0;
>> > +}
>> > +
>> > +csim_unlock_complete (self->priv->csim_lock_task);
>>
>> Reset the csim_lock_task pointer here to NULL, please.
>>
>> > +}
>> > +
>> > +return;
>> > +}
>> > +
>> >  if (cur_qss_status != prev_qss_status)
>> > -mm_dbg ("QSS: status changed '%s -> %s",
>> > +mm_dbg ("QSS handler: status changed '%s -> %s",
>> >  mm_telit_qss_status_get_string (prev_qss_status),
>> >  mm_telit_qss_status_get_string (cur_qss_status));
>> >
>> >  if ((prev_qss_status == QSS_STATUS_SIM_REMOVED && cur_qss_status
>> != QSS_STATUS_SIM_REMOVED) ||
>> >  (prev_qss_status > QSS_STATUS_SIM_REMOVED && cur_qss_status ==
>> QSS_STATUS_SIM_REMOVED)) {
>> > -mm_info ("QSS: SIM swap detected");
>> > +mm_info ("QSS handler: SIM swap detected");
>> >  mm_broadband_modem_update_sim_hot_swap_detected
>> (MM_BROADBAND_MODEM (self));
>> >  }
>> >  }
>> > @@ -610,8 +638,9 @@ csim_unlock_ready (MMBaseModem  *_self,
>> >  if (!response) {