Re: [PATCH] telit: support QMI and MBIM modems

2017-03-27 Thread David McCullough 


Aleksander Morgado wrote the following:
> On Mon, Mar 27, 2017 at 12:47 PM, David McCullough
>  wrote:
> > I am a little late to the party but here is the patch I have been running
> > to do this.  I have been meaning to clean it up and send it in.  Not sure
> > if there is anything here that will help out but I figured it can't hurt :-)
> 
> This is more or less the MBIM part of what we're suggesting to do but
> on mm-1-6, right? In git master we no longer require the TELIT_TAGGED
> tag, and we have the "mm_kernel_device" support instead of the
> GUDevDevice.
> 
> David, if you could test your setup with git master (it's compatible
> to 1.6.x) and Daniele's updated patch, it would be great.

I would love to.  I have been trying to move to master for some time now but
on our platform there is a severe memory corruption in later versions that has 
been very
hard to track down.

One of the object initialisers is overwriting memory in the wrong part of an 
object and
causing MM to crash.  I did know where is was (and who) but I was pulled off it 
for
higher priority issues and it sbeen a while now :-(

Any thoughts on what could be causing glib object iitialisers to get out of 
sync would
be appreciated.  IIRC is was in and around the SIM creation and all in egenric 
code.
Doesn't seem to matter which Modem I am using it will crash.  Eventually I will 
be
free'd up to resolve this, hopefully sooner rather than later ;-)

As for the Telit, it would be nice if we could pull in some of the Telit 
plugins AT
command based methods to supplement the MBIM interface.  I couldn't see any 
example of
that but any advice on the best approach would be appreciated.

Cheers,
Davidm

-- 
David McCullough,  david.mccullo...@accelerated.com,   Ph: 0410 560 763
___
ModemManager-devel mailing list
ModemManager-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/modemmanager-devel

Re: Flow control settings for RS232 modems

2017-03-27 Thread Aleksander Morgado 
On Mon, Mar 27, 2017 at 6:04 PM, carlo  wrote:
>> Does this work?
>>
>> # cat < /lib/udev/rules.d/78-mm-serial.rules
>> ACTION!="add|change|move", GOTO="mm_serial_end"
>> DEVPATH=”/devices/pnp0/00:05/tty/ttyS0”
>> ENV{ID_MM_PLATFORM_DRIVER_PROBE}=”1”
>> LABEL=”mm_serial_end"
>> EOF
>> # udevadm control --reload
>> # udevadm trigger
>> // Relaunch MM
>
> unfortunately no, I got the same result

Can you run "udevadm info -p /sys/class/tty/ttyS0" (i.e. without
--atribute-walk)

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

No SIM card inserted

2017-03-27 Thread Jan Graczyk 
Hello All,

I am working on a development board which has mini PCIe based QUECTEL UC20 
modem card. Unfortunately the modem AT command AT+CPIN? returns SIM not 
inserted. Anyone has come across this problem? Thank you for your support.

Best Regards,

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

Re: Flow control settings for RS232 modems

2017-03-27 Thread carlo 

>  Does this work?


# cat < /lib/udev/rules.d/78-mm-serial.rules
ACTION!="add|change|move", GOTO="mm_serial_end"
DEVPATH=”/devices/pnp0/00:05/tty/ttyS0” ENV{ID_MM_PLATFORM_DRIVER_PROBE}=”1”
LABEL=”mm_serial_end"
EOF
# udevadm control --reload
# udevadm trigger
// Relaunch MM


unfortunately no, I got the same result

Carlo
On 27/03/2017 17:38, Aleksander Morgado wrote:

On Mon, Mar 27, 2017 at 3:52 PM, Carlo Lobrano  wrote:

I've got some problems configuring MM for this test. This is the message I
got just after applying the patch:

ModemManager[28005]:  [1490621030.009049] [mm-base-manager.c:263]
device_added(): (tty/ttyS0): adding device at sysfs path:
/sys/devices/pnp0/00:05/tty/ttyS0
ModemManager[28005]:  [1490621030.009100]
[kerneldevice/mm-kernel-device-udev.c:474] kernel_device_is_candidate():
(tty/ttyS0): port's parent platform driver is not whitelisted

I saw the other email about udev rule to whitelist the serial device, but it
seems I could not get it right.

This is the udevadm info about the serial that responds to AT commands

$ udevadm info --attribute-walk /sys/class/tty/ttyS0

Udevadm info starts with the device specified by the devpath and then
walks up the chain of parent devices. It prints for every device
found, all possible attributes in the udev rules key format.
A rule to match, can be composed by the attributes of the device
and the attributes from one single parent device.

   looking at device '/devices/pnp0/00:05/tty/ttyS0':
 KERNEL=="ttyS0"
 SUBSYSTEM=="tty"
 DRIVER==""

   looking at parent device '/devices/pnp0/00:05':
 KERNELS=="00:05"
 SUBSYSTEMS=="pnp"
 DRIVERS=="serial"
 ATTRS{id}=="PNP0501"

   looking at parent device '/devices/pnp0':
 KERNELS=="pnp0"
 SUBSYSTEMS==""
 DRIVERS==""

I tried adding the line

 DRIVERS=="serial", ENV{ID_MM_PLATFORM_DRIVER_PROBE}="1"

to 77-mm-platform-serial-whitelist.rules and reloading the rules, but it
doesn't seem the correct way to do it.

What am I doing wrong?

Does this work?

# cat < /lib/udev/rules.d/78-mm-serial.rules
ACTION!="add|change|move", GOTO="mm_serial_end"
DEVPATH=”/devices/pnp0/00:05/tty/ttyS0” ENV{ID_MM_PLATFORM_DRIVER_PROBE}=”1”
LABEL=”mm_serial_end"
EOF
# udevadm control --reload
# udevadm trigger
// Relaunch MM




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

[PATCH v4] Fixed wrong MEM1 value passed to +CPMS

2017-03-27 Thread Carlo Lobrano 
In functions

- mm_broadband_modem_lock_sms_storages
- modem_messaging_set_default_storage

return with error when using current_sms_mem1_storage as +CPMS value, but
current_sms_mem1_storage value is UNKNOWN.

---

>> +g_simple_async_report_error_in_idle (
>
>report_error_in_idle() shouldn't be used when there is already a
>GSimpleAsyncResult created in the context. You should instead:
>
>g_simple_async_result_set_error (ctx->result, MM_CORE_ERROR...);
>
>> +G_OBJECT (self),
>> +callback,
>> +user_data,
>> +MM_CORE_ERROR,
>> +MM_CORE_ERROR_RETRY,
>> +"Unknown SMS mem1 storage is allowed only if the current 
>> sms mem1 storage value is not unknown.");
>
>This message is a bit hard to get... maybe something like "cannot lock
>mem2 storage alone when current mem1 storage is unknown"?
>
>> +lock_sms_storages_context_complete_and_free (ctx);
>
>And as we're completing the GSimpleAsyncResult directly in the
>mm_broadband_modem_lock_sms_storages() method, the
>lock_sms_storages_context_complete_and_free() method should be updated
>so that the completion is done in idle (i.e.
>g_simple_async_result_complete_in_idle().

Hope this is right this time. I used "list_call_context_complete_and_free" as 
example.


>> +"Unknown SMS mem1 storage is allowed only if the current 
>> sms mem1 storage value is not unknown.");
>
>This message is a bit hard to get... maybe something like "cannot lock
>mem2 storage alone when current mem1 storage is unknown"?

I wasn't totally happy with them myself. I accept your suggestions.

Sorry for missing the whitespace twice. Btw, do you have any automatic tool for 
checking the coding rules or just a good eye? :D

---
 src/mm-broadband-modem.c   | 77 +-
 src/mm-iface-modem-messaging.c |  2 +-
 2 files changed, 47 insertions(+), 32 deletions(-)

diff --git a/src/mm-broadband-modem.c b/src/mm-broadband-modem.c
index 302fc3d..59bffe5 100644
--- a/src/mm-broadband-modem.c
+++ b/src/mm-broadband-modem.c
@@ -5419,7 +5419,7 @@ typedef struct {
 static void
 lock_sms_storages_context_complete_and_free (LockSmsStoragesContext *ctx)
 {
-g_simple_async_result_complete (ctx->result);
+g_simple_async_result_complete_in_idle (ctx->result);
 g_object_unref (ctx->result);
 g_object_unref (ctx->self);
 g_slice_free (LockSmsStoragesContext, ctx);
@@ -5459,7 +5459,7 @@ mm_broadband_modem_lock_sms_storages (MMBroadbandModem 
*self,
   gpointer user_data)
 {
 LockSmsStoragesContext *ctx;
-gchar *cmd;
+gchar *cmd = NULL;
 gchar *mem1_str = NULL;
 gchar *mem2_str = NULL;
 
@@ -5488,45 +5488,52 @@ mm_broadband_modem_lock_sms_storages (MMBroadbandModem 
*self,
  user_data,
  
mm_broadband_modem_lock_sms_storages);
 
+/* Some modems may not support empty string parameters, then if mem1 is
+ * UNKNOWN, and current sms mem1 storage has a valid value, we send again
+ * the already locked mem1 value in place of an empty string.
+ * This way we also avoid to confuse the environment of other sync 
operation
+ * that have potentially locked mem1 previoulsy.
+ */
 if (mem1 != MM_SMS_STORAGE_UNKNOWN) {
 ctx->mem1_locked = TRUE;
 ctx->previous_mem1 = self->priv->current_sms_mem1_storage;
-self->priv->mem1_storage_locked = TRUE;
+
 self->priv->current_sms_mem1_storage = mem1;
-mem1_str = g_ascii_strup (mm_sms_storage_get_string 
(self->priv->current_sms_mem1_storage), -1);
+self->priv->mem1_storage_locked = TRUE;
+} else if (self->priv->current_sms_mem1_storage != MM_SMS_STORAGE_UNKNOWN) 
{
+mm_dbg ("Given sms mem1 storage is unknown. Using current sms mem1 
storage value '%s' instead",
+mm_sms_storage_get_string 
(self->priv->current_sms_mem1_storage));
+} else {
+g_simple_async_result_set_error (ctx->result,
+ MM_CORE_ERROR,
+ MM_CORE_ERROR_RETRY,
+"Cannot lock mem2 storage alone when 
current mem1 storage is unknown");
+lock_sms_storages_context_complete_and_free (ctx);
+return;
 }
+mem1_str = g_ascii_strup (mm_sms_storage_get_string 
(self->priv->current_sms_mem1_storage), -1);
 
 if (mem2 != MM_SMS_STORAGE_UNKNOWN) {
 ctx->mem2_locked = TRUE;
 ctx->previous_mem2 = self->priv->current_sms_mem2_storage;
+
 self->priv->mem2_storage_locked = TRUE;
 self->priv->current_sms_mem2_storage = mem2;
-mem2_str = g_ascii_strup (mm_sms_storage_get_string 
(self->priv->current_sms_mem2_storage), -1);
 
-if (mem1 == MM_SMS_STORAGE_UNKNOWN) {
-/* Some modems may not support empty string

Re: [PATCH] telit: support QMI and MBIM modems

2017-03-27 Thread Aleksander Morgado 
> Vendor specific plugins that support QMI or MBIM based devices need to
> handle the creation of these modems themselves.
>
> https://bugs.freedesktop.org/show_bug.cgi?id=100372
> ---
>
> Hey Carlo and Daniele,
>
> This patch makes the Telit plugin accept QMI and MBIM modems. Can any of 
> you test it with such modems to make sure the Telit plugin is the one 
> grabbing them?
>

 Sure, I can give it a try on Monday.

>>>
>>> Applying your patch and testing with an mbim based I still see that
>>> the generic plugin is used
>>>
>>> daniele@L2122:~/git/ModemManager$ mmcli -L
>>>
>>> Found 1 modems:
>>> /org/freedesktop/ModemManager1/Modem/0 [Generic] MBIM [1BC7:0032]
>>>
>>> So in the log I found
>>>
>>> ModemManager[3807]:  (Telit) [cdc-wdm0] filtered by implicit MBIM 
>>> driver
>>>
>>> and added
>>>
>>>   MM_PLUGIN_ALLOWED_QMI,TRUE,
>>>   MM_PLUGIN_ALLOWED_MBIM,   TRUE,
>>>
>>> in mm_plugin_create.
>>>
>>
>> Ouch, yes, that is needed.
>>
>>> I'm now seeing
>>>
>>> ModemManager[5758]:  MBIM-powered Telit modem found...
>>>
>>> but also
>>>
>>> ModemManager[5758]:   Couldn't start initialization: Cannot
>>> initialize: MBIM port went missing
>>> ModemManager[5758]:   couldn't initialize the modem: 'Modem is
>>> unusable, cannot fully initialize'
>>>
>>> and the modem was not recognized.
>>>
>>
>> That is very weird. Do you have the full log around?
>>
>
> I did a new one with a qmi based device, but basically it is the same,
> you can find it here:
>
> https://pastebin.com/rqWhmgSR
>
> Mar 27 16:37:08 L2122 ModemManager[21199]:   Couldn't start
> initialization: Cannot initialize: QMI port went missing
> 
> Mar 27 16:37:08 L2122 ModemManager[21199]:   couldn't initialize
> the modem: 'Modem is unusable, cannot fully initialize'
>
> This does not happen if I fix telit_grab_port.
>

Ah, so the issue doesn't happen either in MBIM or QMI if you fix
telit_grab_port() as you did? That is perfectly fine for me, actually
I guess that if grab_port() isn't fixed we're just flagging the port
as MM_PORT_TYPE_IGNORED, which is why you get the issue.

>>> I had also to fix telit_grab_port in order to take mbim, qmi and net
>>> ports. Does this make sense?
>>
>> Yes, it also makes sense...
>>
>> Could you share your latest additions and the full log you're getting?
>>
>
> Attached you can find the updated patch that just skips the port
> identification if the subsystem is not tty: not sure if this is the
> best approach...
>

Yes, that approach is fine I think; it should cover all the cases
where you do want port type hints (AT primary, AT secondary and NMEA).

So all looking good from my POV.

David, Dan, what do you guys think?

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

Re: Try to normalize both operator name and code

2017-03-27 Thread Colin Helliwell 

> On 25 March 2017 at 20:39 Aleksander Morgado  wrote:
> 
> Hey Colin and Dan,
> 
> What do you think of these two patches?
> 
> Cheers!
> 
> [PATCH 1/2] modem-helpers: if operator not in UCS2, see if already
> [PATCH 2/2] broadband-modem: normalize also operator code
> 

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

Re: [PATCH v3] Fixed wrong MEM1 value passed to +CPMS

2017-03-27 Thread Aleksander Morgado 
Hey Carlo,

On 27/03/17 12:19, Carlo Lobrano wrote:
> In functions
> 
> - mm_broadband_modem_lock_sms_storages
> - modem_messaging_set_default_storage
> 
> return with error when using current_sms_mem1_storage as +CPMS value, but
> current_sms_mem1_storage value is UNKNOWN.
> 

See comments below.

> ---
> 
>  src/mm-broadband-modem.c   | 79 
> ++
>  src/mm-iface-modem-messaging.c |  2 +-
>  2 files changed, 50 insertions(+), 31 deletions(-)
> 
> diff --git a/src/mm-broadband-modem.c b/src/mm-broadband-modem.c
> index 302fc3d..c15f18d 100644
> --- a/src/mm-broadband-modem.c
> +++ b/src/mm-broadband-modem.c
> @@ -5459,7 +5459,7 @@ mm_broadband_modem_lock_sms_storages (MMBroadbandModem 
> *self,
>gpointer user_data)
>  {
>  LockSmsStoragesContext *ctx;
> -gchar *cmd;
> +gchar *cmd = NULL;
>  gchar *mem1_str = NULL;
>  gchar *mem2_str = NULL;
>  
> @@ -5488,45 +5488,55 @@ mm_broadband_modem_lock_sms_storages 
> (MMBroadbandModem *self,
>   user_data,
>   
> mm_broadband_modem_lock_sms_storages);
>  
> +/* Some modems may not support empty string parameters, then if mem1 is
> + * UNKNOWN, and current sms mem1 storage has a valid value, we send again
> + * the already locked mem1 value in place of an empty string.
> + * This way we also avoid to confuse the environment of other sync 
> operation
> + * that have potentially locked mem1 previoulsy.
> + */
>  if (mem1 != MM_SMS_STORAGE_UNKNOWN) {
>  ctx->mem1_locked = TRUE;
>  ctx->previous_mem1 = self->priv->current_sms_mem1_storage;
> -self->priv->mem1_storage_locked = TRUE;
> +
>  self->priv->current_sms_mem1_storage = mem1;
> -mem1_str = g_ascii_strup (mm_sms_storage_get_string 
> (self->priv->current_sms_mem1_storage), -1);
> +self->priv->mem1_storage_locked = TRUE;
> +} else if (self->priv->current_sms_mem1_storage != 
> MM_SMS_STORAGE_UNKNOWN) {
> +mm_dbg ("Given sms mem1 storage is unknown. Using current sms mem1 
> storage value '%s' instead",
> +mm_sms_storage_get_string 
> (self->priv->current_sms_mem1_storage));
> +} else {
> +g_simple_async_report_error_in_idle (

report_error_in_idle() shouldn't be used when there is already a
GSimpleAsyncResult created in the context. You should instead:

g_simple_async_result_set_error (ctx->result, MM_CORE_ERROR...);

> +G_OBJECT (self),
> +callback,
> +user_data,
> +MM_CORE_ERROR,
> +MM_CORE_ERROR_RETRY,
> +"Unknown SMS mem1 storage is allowed only if the current sms 
> mem1 storage value is not unknown.");

This message is a bit hard to get... maybe something like "cannot lock
mem2 storage alone when current mem1 storage is unknown"?

> +lock_sms_storages_context_complete_and_free (ctx);

And as we're completing the GSimpleAsyncResult directly in the
mm_broadband_modem_lock_sms_storages() method, the
lock_sms_storages_context_complete_and_free() method should be updated
so that the completion is done in idle (i.e.
g_simple_async_result_complete_in_idle().


> +return;
>  }
> +mem1_str = g_ascii_strup (mm_sms_storage_get_string 
> (self->priv->current_sms_mem1_storage), -1);
>  
>  if (mem2 != MM_SMS_STORAGE_UNKNOWN) {
>  ctx->mem2_locked = TRUE;
>  ctx->previous_mem2 = self->priv->current_sms_mem2_storage;
> +
>  self->priv->mem2_storage_locked = TRUE;
>  self->priv->current_sms_mem2_storage = mem2;
> -mem2_str = g_ascii_strup (mm_sms_storage_get_string 
> (self->priv->current_sms_mem2_storage), -1);
>  
> -if (mem1 == MM_SMS_STORAGE_UNKNOWN) {
> -/* Some modems may not support empty string parameters. Then if 
> mem1 is
> - * UNKNOWN, we send again the already locked mem1 value in place 
> of an
> - * empty string. This way we also avoid to confuse the 
> environment of
> - * other async operation that have potentially locked mem1 
> previoulsy.
> - * */
> -mem1_str = g_ascii_strup (mm_sms_storage_get_string 
> (self->priv->current_sms_mem1_storage), -1);
> -}
> +mem2_str = g_ascii_strup (mm_sms_storage_get_string 
> (self->priv->current_sms_mem2_storage), -1);
>  }
>  
> -/* We don't touch 'mem3' here */
> +g_assert(mem1_str != NULL);
>  

Whitespace missing before the parenthesis.

> +/* We don't touch 'mem3' here */
>  mm_dbg ("Locking SMS storages to: mem1 (%s), mem2 (%s)...",
> -mem1_str ? mem1_str : "none",
> +mem1_str,
>  mem2_str ? mem2_str : "none");
>  
>  if (mem2_str)
> -cmd = g_strdup_printf ("+CPMS=\"%s\",\"%s\"",
> -   mem1_str ? mem1_str : "",

Re: [PATCH] telit: support QMI and MBIM modems

2017-03-27 Thread Aleksander Morgado 
On Mon, Mar 27, 2017 at 12:47 PM, David McCullough
 wrote:
> I am a little late to the party but here is the patch I have been running
> to do this.  I have been meaning to clean it up and send it in.  Not sure
> if there is anything here that will help out but I figured it can't hurt :-)

This is more or less the MBIM part of what we're suggesting to do but
on mm-1-6, right? In git master we no longer require the TELIT_TAGGED
tag, and we have the "mm_kernel_device" support instead of the
GUDevDevice.

David, if you could test your setup with git master (it's compatible
to 1.6.x) and Daniele's updated patch, it would be great.

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

Re: [PATCH] telit: support QMI and MBIM modems

2017-03-27 Thread Aleksander Morgado 
On Mon, Mar 27, 2017 at 12:14 PM, Daniele Palmas  wrote:
> 2017-03-24 15:33 GMT+01:00 Daniele Palmas :
>> Hi Aleksander,
>>
>> 2017-03-24 14:48 GMT+01:00 Aleksander Morgado :
>>> Vendor specific plugins that support QMI or MBIM based devices need to
>>> handle the creation of these modems themselves.
>>>
>>> https://bugs.freedesktop.org/show_bug.cgi?id=100372
>>> ---
>>>
>>> Hey Carlo and Daniele,
>>>
>>> This patch makes the Telit plugin accept QMI and MBIM modems. Can any of 
>>> you test it with such modems to make sure the Telit plugin is the one 
>>> grabbing them?
>>>
>>
>> Sure, I can give it a try on Monday.
>>
>
> Applying your patch and testing with an mbim based I still see that
> the generic plugin is used
>
> daniele@L2122:~/git/ModemManager$ mmcli -L
>
> Found 1 modems:
> /org/freedesktop/ModemManager1/Modem/0 [Generic] MBIM [1BC7:0032]
>
> So in the log I found
>
> ModemManager[3807]:  (Telit) [cdc-wdm0] filtered by implicit MBIM 
> driver
>
> and added
>
>   MM_PLUGIN_ALLOWED_QMI,TRUE,
>   MM_PLUGIN_ALLOWED_MBIM,   TRUE,
>
> in mm_plugin_create.
>

Ouch, yes, that is needed.

> I'm now seeing
>
> ModemManager[5758]:  MBIM-powered Telit modem found...
>
> but also
>
> ModemManager[5758]:   Couldn't start initialization: Cannot
> initialize: MBIM port went missing
> ModemManager[5758]:   couldn't initialize the modem: 'Modem is
> unusable, cannot fully initialize'
>
> and the modem was not recognized.
>

That is very weird. Do you have the full log around?

> I had also to fix telit_grab_port in order to take mbim, qmi and net
> ports. Does this make sense?

Yes, it also makes sense...

Could you share your latest additions and the full log you're getting?


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

[PATCH v3] Fixed wrong MEM1 value passed to +CPMS

2017-03-27 Thread Carlo Lobrano 
In functions

- mm_broadband_modem_lock_sms_storages
- modem_messaging_set_default_storage

return with error when using current_sms_mem1_storage as +CPMS value, but
current_sms_mem1_storage value is UNKNOWN.

---

 src/mm-broadband-modem.c   | 79 ++
 src/mm-iface-modem-messaging.c |  2 +-
 2 files changed, 50 insertions(+), 31 deletions(-)

diff --git a/src/mm-broadband-modem.c b/src/mm-broadband-modem.c
index 302fc3d..c15f18d 100644
--- a/src/mm-broadband-modem.c
+++ b/src/mm-broadband-modem.c
@@ -5459,7 +5459,7 @@ mm_broadband_modem_lock_sms_storages (MMBroadbandModem 
*self,
   gpointer user_data)
 {
 LockSmsStoragesContext *ctx;
-gchar *cmd;
+gchar *cmd = NULL;
 gchar *mem1_str = NULL;
 gchar *mem2_str = NULL;
 
@@ -5488,45 +5488,55 @@ mm_broadband_modem_lock_sms_storages (MMBroadbandModem 
*self,
  user_data,
  
mm_broadband_modem_lock_sms_storages);
 
+/* Some modems may not support empty string parameters, then if mem1 is
+ * UNKNOWN, and current sms mem1 storage has a valid value, we send again
+ * the already locked mem1 value in place of an empty string.
+ * This way we also avoid to confuse the environment of other sync 
operation
+ * that have potentially locked mem1 previoulsy.
+ */
 if (mem1 != MM_SMS_STORAGE_UNKNOWN) {
 ctx->mem1_locked = TRUE;
 ctx->previous_mem1 = self->priv->current_sms_mem1_storage;
-self->priv->mem1_storage_locked = TRUE;
+
 self->priv->current_sms_mem1_storage = mem1;
-mem1_str = g_ascii_strup (mm_sms_storage_get_string 
(self->priv->current_sms_mem1_storage), -1);
+self->priv->mem1_storage_locked = TRUE;
+} else if (self->priv->current_sms_mem1_storage != MM_SMS_STORAGE_UNKNOWN) 
{
+mm_dbg ("Given sms mem1 storage is unknown. Using current sms mem1 
storage value '%s' instead",
+mm_sms_storage_get_string 
(self->priv->current_sms_mem1_storage));
+} else {
+g_simple_async_report_error_in_idle (
+G_OBJECT (self),
+callback,
+user_data,
+MM_CORE_ERROR,
+MM_CORE_ERROR_RETRY,
+"Unknown SMS mem1 storage is allowed only if the current sms 
mem1 storage value is not unknown.");
+lock_sms_storages_context_complete_and_free (ctx);
+return;
 }
+mem1_str = g_ascii_strup (mm_sms_storage_get_string 
(self->priv->current_sms_mem1_storage), -1);
 
 if (mem2 != MM_SMS_STORAGE_UNKNOWN) {
 ctx->mem2_locked = TRUE;
 ctx->previous_mem2 = self->priv->current_sms_mem2_storage;
+
 self->priv->mem2_storage_locked = TRUE;
 self->priv->current_sms_mem2_storage = mem2;
-mem2_str = g_ascii_strup (mm_sms_storage_get_string 
(self->priv->current_sms_mem2_storage), -1);
 
-if (mem1 == MM_SMS_STORAGE_UNKNOWN) {
-/* Some modems may not support empty string parameters. Then if 
mem1 is
- * UNKNOWN, we send again the already locked mem1 value in place 
of an
- * empty string. This way we also avoid to confuse the environment 
of
- * other async operation that have potentially locked mem1 
previoulsy.
- * */
-mem1_str = g_ascii_strup (mm_sms_storage_get_string 
(self->priv->current_sms_mem1_storage), -1);
-}
+mem2_str = g_ascii_strup (mm_sms_storage_get_string 
(self->priv->current_sms_mem2_storage), -1);
 }
 
-/* We don't touch 'mem3' here */
+g_assert(mem1_str != NULL);
 
+/* We don't touch 'mem3' here */
 mm_dbg ("Locking SMS storages to: mem1 (%s), mem2 (%s)...",
-mem1_str ? mem1_str : "none",
+mem1_str,
 mem2_str ? mem2_str : "none");
 
 if (mem2_str)
-cmd = g_strdup_printf ("+CPMS=\"%s\",\"%s\"",
-   mem1_str ? mem1_str : "",
-   mem2_str);
-else if (mem1_str)
-cmd = g_strdup_printf ("+CPMS=\"%s\"", mem1_str);
+cmd = g_strdup_printf ("+CPMS=\"%s\",\"%s\"", mem1_str, mem2_str);
 else
-g_assert_not_reached ();
+cmd = g_strdup_printf ("+CPMS=\"%s\"", mem1_str);
 
 mm_base_modem_at_command (MM_BASE_MODEM (self),
   cmd,
@@ -5578,22 +5588,31 @@ modem_messaging_set_default_storage 
(MMIfaceModemMessaging *_self,
 gchar *mem1_str;
 gchar *mem_str;
 
-result = g_simple_async_result_new (G_OBJECT (self),
-callback,
-user_data,
-modem_messaging_set_default_storage);
+/* We provide the current sms storage for mem1 if not UNKNOWN */
+if (self->priv->current_sms_mem1_storage == MM_SMS_STORAGE_UNKNOWN) {
+

Re: [PATCH] telit: support QMI and MBIM modems

2017-03-27 Thread Daniele Palmas 
Hi Aleksander,

2017-03-24 15:33 GMT+01:00 Daniele Palmas :
> Hi Aleksander,
>
> 2017-03-24 14:48 GMT+01:00 Aleksander Morgado :
>> Vendor specific plugins that support QMI or MBIM based devices need to
>> handle the creation of these modems themselves.
>>
>> https://bugs.freedesktop.org/show_bug.cgi?id=100372
>> ---
>>
>> Hey Carlo and Daniele,
>>
>> This patch makes the Telit plugin accept QMI and MBIM modems. Can any of you 
>> test it with such modems to make sure the Telit plugin is the one grabbing 
>> them?
>>
>
> Sure, I can give it a try on Monday.
>

Applying your patch and testing with an mbim based I still see that
the generic plugin is used

daniele@L2122:~/git/ModemManager$ mmcli -L

Found 1 modems:
/org/freedesktop/ModemManager1/Modem/0 [Generic] MBIM [1BC7:0032]

So in the log I found

ModemManager[3807]:  (Telit) [cdc-wdm0] filtered by implicit MBIM driver

and added

  MM_PLUGIN_ALLOWED_QMI,TRUE,
  MM_PLUGIN_ALLOWED_MBIM,   TRUE,

in mm_plugin_create.

I'm now seeing

ModemManager[5758]:  MBIM-powered Telit modem found...

but also

ModemManager[5758]:   Couldn't start initialization: Cannot
initialize: MBIM port went missing
ModemManager[5758]:   couldn't initialize the modem: 'Modem is
unusable, cannot fully initialize'

and the modem was not recognized.

I had also to fix telit_grab_port in order to take mbim, qmi and net
ports. Does this make sense?

Daniele

>> Cheers!
>>
>> ---
>>  plugins/telit/77-mm-telit-port-types.rules |  3 ---
>>  plugins/telit/mm-plugin-telit.c| 33 
>> +-
>>  2 files changed, 32 insertions(+), 4 deletions(-)
>>
>> diff --git a/plugins/telit/77-mm-telit-port-types.rules 
>> b/plugins/telit/77-mm-telit-port-types.rules
>> index 36a4f99f..1b58a3d9 100644
>> --- a/plugins/telit/77-mm-telit-port-types.rules
>> +++ b/plugins/telit/77-mm-telit-port-types.rules
>> @@ -51,7 +51,4 @@ ATTRS{idVendor}=="1bc7", ATTRS{idProduct}=="0022", 
>> ENV{ID_MM_TELIT_PORTS_TAGGED}
>>  ATTRS{idVendor}=="1bc7", ATTRS{idProduct}=="0036", 
>> ENV{ID_MM_TELIT_TAGGED}="1"
>>  ATTRS{idVendor}=="1bc7", ATTRS{idProduct}=="0036", 
>> ENV{ID_MM_TELIT_PORTS_TAGGED}="1"
>>
>> -# NOTE: Qualcomm Gobi-based devices like the LE920 should not be handled
>> -# by this plugin, but by the Gobi plugin.
>> -
>>  LABEL="mm_telit_port_types_end"
>> diff --git a/plugins/telit/mm-plugin-telit.c 
>> b/plugins/telit/mm-plugin-telit.c
>> index ec3c024f..abb87e4f 100644
>> --- a/plugins/telit/mm-plugin-telit.c
>> +++ b/plugins/telit/mm-plugin-telit.c
>> @@ -28,6 +28,15 @@
>>  #include "mm-common-telit.h"
>>  #include "mm-broadband-modem-telit.h"
>>
>> +
>> +#if defined WITH_QMI
>> +# include "mm-broadband-modem-qmi.h"
>> +#endif
>> +
>> +#if defined WITH_MBIM
>> +# include "mm-broadband-modem-mbim.h"
>> +#endif
>> +
>>  G_DEFINE_TYPE (MMPluginTelit, mm_plugin_telit, MM_TYPE_PLUGIN)
>>
>>  MM_PLUGIN_DEFINE_MAJOR_VERSION
>> @@ -44,6 +53,28 @@ create_modem (MMPlugin *self,
>>GList *probes,
>>GError **error)
>>  {
>> +#if defined WITH_QMI
>> +if (mm_port_probe_list_has_qmi_port (probes)) {
>> +mm_dbg ("QMI-powered Telit modem found...");
>> +return MM_BASE_MODEM (mm_broadband_modem_qmi_new (uid,
>> +  drivers,
>> +  
>> mm_plugin_get_name (self),
>> +  vendor,
>> +  product));
>> +}
>> +#endif
>> +
>> +#if defined WITH_MBIM
>> +if (mm_port_probe_list_has_mbim_port (probes)) {
>> +mm_dbg ("MBIM-powered Telit modem found...");
>> +return MM_BASE_MODEM (mm_broadband_modem_mbim_new (uid,
>> +   drivers,
>> +   
>> mm_plugin_get_name (self),
>> +   vendor,
>> +   product));
>> +}
>> +#endif
>> +
>>  return MM_BASE_MODEM (mm_broadband_modem_telit_new (uid,
>>  drivers,
>>  mm_plugin_get_name 
>> (self),
>> @@ -56,7 +87,7 @@ create_modem (MMPlugin *self,
>>  G_MODULE_EXPORT MMPlugin *
>>  mm_plugin_create (void)
>>  {
>> -static const gchar *subsystems[] = { "tty", NULL };
>> +static const gchar *subsystems[] = { "tty", "net", "usb", NULL };
>>  /* Vendors: Telit */
>>  static const guint16 vendor_ids[] = { 0x1bc7, 0 };
>>  static const gchar *vendor_strings[] = { "telit", NULL };
>> --
>> 2.12.0
___
ModemManager-devel mailing list
ModemManager-devel@lists.freedesktop.org