Re: [review] https://github.com/cbchan/ModemManager/tree/modem-hardware-revision
On Mon, Sep 18, 2017 at 8:13 AM, Ben Chan wrote: > Similar to firmware revision, hardware revision is useful information > for identifying characteristics about a modem module. Both MBIM and > QMI provides API to query hardware revision from the modem. This > series of patch add support of loading and reporting hardware revision > through the Modem interface. > > https://github.com/linux-mobile-broadband/ModemManager/compare/master...cbchan:modem-hardware-revision They all LGTM. Dan, what do you think? -- Aleksander https://aleksander.es ___ ModemManager-devel mailing list ModemManager-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/modemmanager-devel
Re: [PATCH 2/2] broadband-modem-mbim: fix preservation logic for PIN1 unlock retries
On Mon, Sep 18, 2017 at 9:10 AM, Ben Chan wrote:
> This patches fixes commit 334273979 "broadband-modem-mbim: preserve
> unlock retries for PIN1 when appropriate", which doesn't correctly
> propagate the unlock retries information for PIN1 observed from
> responses to MBIM_CID_PIN set operations (see commit eb9ec1b61
> "sim-mbim: update unlock retries information after PIN operations").
> ---
LGTM
Dan, what do you think? Given that the PIN retries are updated in the
iface from within different places (e.g. modem object and sim object)
it probably isn't a good thing to cache the value anywhere else than
in the iface itself. This is probably the simplest solution.
> src/mm-broadband-modem-mbim.c | 29 +
> 1 file changed, 13 insertions(+), 16 deletions(-)
>
> diff --git a/src/mm-broadband-modem-mbim.c b/src/mm-broadband-modem-mbim.c
> index 93a61085..aaa3e1cc 100644
> --- a/src/mm-broadband-modem-mbim.c
> +++ b/src/mm-broadband-modem-mbim.c
> @@ -93,9 +93,6 @@ struct _MMBroadbandModemMbimPrivate {
>
> /* For notifying when the mbim-proxy connection is dead */
> gulong mbim_device_removed_id;
> -
> -/* Previously observed SIM-PIN remaining retries */
> -guint sim_pin_retries;
> };
>
>
> /*/
> @@ -713,7 +710,7 @@ pin_query_unlock_retries_ready (MbimDevice *device,
> NULL,
> &remaining_attempts,
> &error)) {
> -MMBroadbandModemMbim *self;
> +MMIfaceModem *self;
> MMModemLock lock;
> MMUnlockRetries *retries;
>
> @@ -737,22 +734,24 @@ pin_query_unlock_retries_ready (MbimDevice *device,
> * PIN1. Here we thus carry over any existing information on PIN1
> from
> * MMIfaceModem's MMUnlockRetries if the MBIM_CID_PIN query reports
> * something other than PIN1. */
> -if (lock != MM_MODEM_LOCK_SIM_PIN &&
> -self->priv->sim_pin_retries != MM_UNLOCK_RETRIES_UNKNOWN) {
> -mm_unlock_retries_set (retries,
> - MM_MODEM_LOCK_SIM_PIN,
> - self->priv->sim_pin_retries);
> +if (lock != MM_MODEM_LOCK_SIM_PIN) {
> +MMUnlockRetries *previous_retries;
> +guint previous_sim_pin_retries;
> +
> +previous_retries = mm_iface_modem_get_unlock_retries (self);
> +previous_sim_pin_retries = mm_unlock_retries_get
> (previous_retries,
> +
> MM_MODEM_LOCK_SIM_PIN);
> +if (previous_sim_pin_retries != MM_UNLOCK_RETRIES_UNKNOWN) {
> +mm_unlock_retries_set (retries,
> + MM_MODEM_LOCK_SIM_PIN,
> + previous_sim_pin_retries);
> +}
> }
>
> /* According to the MBIM specification, RemainingAttempts is set to
> * 0x if the device does not support this information. */
> if (remaining_attempts != G_MAXUINT32)
> mm_unlock_retries_set (retries, lock, remaining_attempts);
> -else
> -remaining_attempts = MM_UNLOCK_RETRIES_UNKNOWN;
> -
> -if (lock == MM_MODEM_LOCK_SIM_PIN)
> -self->priv->sim_pin_retries = remaining_attempts;
>
> g_task_return_pointer (task, retries, g_object_unref);
> } else
> @@ -3274,8 +3273,6 @@ mm_broadband_modem_mbim_init (MMBroadbandModemMbim
> *self)
> self->priv = G_TYPE_INSTANCE_GET_PRIVATE (self,
>MM_TYPE_BROADBAND_MODEM_MBIM,
>MMBroadbandModemMbimPrivate);
> -
> -self->priv->sim_pin_retries = MM_UNLOCK_RETRIES_UNKNOWN;
> }
>
> static void
> --
> 2.14.1.690.gbb1197296e-goog
>
--
Aleksander
https://aleksander.es
___
ModemManager-devel mailing list
ModemManager-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/modemmanager-devel
Re: [PATCH 1/2] iface-modem: add mm_iface_modem_get_unlock_retries helper
On Mon, Sep 18, 2017 at 9:10 AM, Ben Chan wrote:
> This patch adds a mm_iface_modem_get_unlock_retries helper for getting
> the current MMUnlockRetries value of a MMIfaceModem object, which later
> allows us to partially update (e.g. a specific MMModemLock) the
> MMUnlockRetries value of a MMIfaceModem object.
> ---
LGTM
> src/mm-iface-modem.c | 21 +
> src/mm-iface-modem.h | 2 ++
> 2 files changed, 23 insertions(+)
>
> diff --git a/src/mm-iface-modem.c b/src/mm-iface-modem.c
> index e21d5aeb..cc4d6991 100644
> --- a/src/mm-iface-modem.c
> +++ b/src/mm-iface-modem.c
> @@ -2926,6 +2926,27 @@ set_lock_status (MMIfaceModem *self,
> }
> }
>
> +MMUnlockRetries *
> +mm_iface_modem_get_unlock_retries (MMIfaceModem *self)
> +{
> +MmGdbusModem *skeleton = NULL;
> +MMUnlockRetries *unlock_retries;
> +
> +g_object_get (self,
> + MM_IFACE_MODEM_DBUS_SKELETON, &skeleton,
> + NULL);
> +if (skeleton) {
> +GVariant *dictionary;
> +
> +dictionary = mm_gdbus_modem_get_unlock_retries (skeleton);
> +unlock_retries = mm_unlock_retries_new_from_dictionary (dictionary);
> +g_object_unref (skeleton);
> +} else
> +unlock_retries = mm_unlock_retries_new ();
> +
> +return unlock_retries;
> +}
> +
> void
> mm_iface_modem_update_unlock_retries (MMIfaceModem *self,
>MMUnlockRetries *unlock_retries)
> diff --git a/src/mm-iface-modem.h b/src/mm-iface-modem.h
> index af039b3b..1a154b48 100644
> --- a/src/mm-iface-modem.h
> +++ b/src/mm-iface-modem.h
> @@ -414,6 +414,8 @@ MMModemLock mm_iface_modem_update_lock_info_finish
> (MMIfaceModem *self,
> GAsyncResult *res,
> GError **error);
>
> +MMUnlockRetries *mm_iface_modem_get_unlock_retries (MMIfaceModem *self);
> +
> void mm_iface_modem_update_unlock_retries (MMIfaceModem *self,
> MMUnlockRetries *unlock_retries);
>
> --
> 2.14.1.690.gbb1197296e-goog
>
--
Aleksander
https://aleksander.es
___
ModemManager-devel mailing list
ModemManager-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/modemmanager-devel
Re: [review] https://github.com/cbchan/ModemManager/tree/gtask-plugins-sierra
On Sun, Sep 17, 2017 at 6:36 PM, Ben Chan wrote: > This branch contains a series of patches that port the sierra plugin > to use GTask: > > https://github.com/linux-mobile-broadband/ModemManager/compare/master...cbchan:gtask-plugins-sierra All LGTM! Pushed to git master along with some additional minor updates. -- Aleksander https://aleksander.es ___ ModemManager-devel mailing list ModemManager-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/modemmanager-devel
Re: [review] https://github.com/cbchan/ModemManager/tree/gtask-plugins-option
On Sun, Sep 17, 2017 at 6:35 PM, Ben Chan wrote: > This branch contains a series of patches that port the option plugin > to use GTask: > > https://github.com/linux-mobile-broadband/ModemManager/compare/master...cbchan:gtask-plugins-option All LGTM, merged to git master. -- Aleksander https://aleksander.es ___ ModemManager-devel mailing list ModemManager-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/modemmanager-devel
Re: [review] https://github.com/cbchan/ModemManager/tree/sim-mbim-unlock-retries
On Tue, Sep 19, 2017 at 12:01 AM, Aleksander Morgado wrote: >> >> I overlooked one thing when I revised the patches based on Dan's >> suggestion, and also missed that in my testing :-( >> >> MMSimMbim:update_modem_unlock_retries() calls >> mm_iface_modem_update_unlock_retries() to report remaining attempts >> PIN1. That won't affect MMBroadbandModemMbimPriv::sim_pin_retries. My >> original approach was to handle all MMUnlockRetries updates via >> MMIfaceModem. >> >> If we prefer caching PIN1 retries in MMBroadbandModemMbim, >> MMBroadbandModemMbim needs to provide a method (e.g. >> mm_broadband_modem_mbm_set_sim_pin_retries) for MMSimMbim to propagate >> the information. Would you prefer that or should I go back to my >> original approach? >> > > Probably better to go with the old approach I think; I wouldn't like > MMIfaceModem code to call MMBroadbandModemMbim APIs. Uploaded two patches that change the code to use the original approach. Dan, let me know if you've any concern or would like to propose a different approach. I can adjust the patches accordingly. Aleksander, I've done some basic tests on my SIMs. Thanks. > > -- > Aleksander > https://aleksander.es ___ ModemManager-devel mailing list ModemManager-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/modemmanager-devel
[PATCH 2/2] broadband-modem-mbim: fix preservation logic for PIN1 unlock retries
This patches fixes commit 334273979 "broadband-modem-mbim: preserve
unlock retries for PIN1 when appropriate", which doesn't correctly
propagate the unlock retries information for PIN1 observed from
responses to MBIM_CID_PIN set operations (see commit eb9ec1b61
"sim-mbim: update unlock retries information after PIN operations").
---
src/mm-broadband-modem-mbim.c | 29 +
1 file changed, 13 insertions(+), 16 deletions(-)
diff --git a/src/mm-broadband-modem-mbim.c b/src/mm-broadband-modem-mbim.c
index 93a61085..aaa3e1cc 100644
--- a/src/mm-broadband-modem-mbim.c
+++ b/src/mm-broadband-modem-mbim.c
@@ -93,9 +93,6 @@ struct _MMBroadbandModemMbimPrivate {
/* For notifying when the mbim-proxy connection is dead */
gulong mbim_device_removed_id;
-
-/* Previously observed SIM-PIN remaining retries */
-guint sim_pin_retries;
};
/*/
@@ -713,7 +710,7 @@ pin_query_unlock_retries_ready (MbimDevice *device,
NULL,
&remaining_attempts,
&error)) {
-MMBroadbandModemMbim *self;
+MMIfaceModem *self;
MMModemLock lock;
MMUnlockRetries *retries;
@@ -737,22 +734,24 @@ pin_query_unlock_retries_ready (MbimDevice *device,
* PIN1. Here we thus carry over any existing information on PIN1 from
* MMIfaceModem's MMUnlockRetries if the MBIM_CID_PIN query reports
* something other than PIN1. */
-if (lock != MM_MODEM_LOCK_SIM_PIN &&
-self->priv->sim_pin_retries != MM_UNLOCK_RETRIES_UNKNOWN) {
-mm_unlock_retries_set (retries,
- MM_MODEM_LOCK_SIM_PIN,
- self->priv->sim_pin_retries);
+if (lock != MM_MODEM_LOCK_SIM_PIN) {
+MMUnlockRetries *previous_retries;
+guint previous_sim_pin_retries;
+
+previous_retries = mm_iface_modem_get_unlock_retries (self);
+previous_sim_pin_retries = mm_unlock_retries_get (previous_retries,
+
MM_MODEM_LOCK_SIM_PIN);
+if (previous_sim_pin_retries != MM_UNLOCK_RETRIES_UNKNOWN) {
+mm_unlock_retries_set (retries,
+ MM_MODEM_LOCK_SIM_PIN,
+ previous_sim_pin_retries);
+}
}
/* According to the MBIM specification, RemainingAttempts is set to
* 0x if the device does not support this information. */
if (remaining_attempts != G_MAXUINT32)
mm_unlock_retries_set (retries, lock, remaining_attempts);
-else
-remaining_attempts = MM_UNLOCK_RETRIES_UNKNOWN;
-
-if (lock == MM_MODEM_LOCK_SIM_PIN)
-self->priv->sim_pin_retries = remaining_attempts;
g_task_return_pointer (task, retries, g_object_unref);
} else
@@ -3274,8 +3273,6 @@ mm_broadband_modem_mbim_init (MMBroadbandModemMbim *self)
self->priv = G_TYPE_INSTANCE_GET_PRIVATE (self,
MM_TYPE_BROADBAND_MODEM_MBIM,
MMBroadbandModemMbimPrivate);
-
-self->priv->sim_pin_retries = MM_UNLOCK_RETRIES_UNKNOWN;
}
static void
--
2.14.1.690.gbb1197296e-goog
___
ModemManager-devel mailing list
ModemManager-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/modemmanager-devel
[PATCH 1/2] iface-modem: add mm_iface_modem_get_unlock_retries helper
This patch adds a mm_iface_modem_get_unlock_retries helper for getting
the current MMUnlockRetries value of a MMIfaceModem object, which later
allows us to partially update (e.g. a specific MMModemLock) the
MMUnlockRetries value of a MMIfaceModem object.
---
src/mm-iface-modem.c | 21 +
src/mm-iface-modem.h | 2 ++
2 files changed, 23 insertions(+)
diff --git a/src/mm-iface-modem.c b/src/mm-iface-modem.c
index e21d5aeb..cc4d6991 100644
--- a/src/mm-iface-modem.c
+++ b/src/mm-iface-modem.c
@@ -2926,6 +2926,27 @@ set_lock_status (MMIfaceModem *self,
}
}
+MMUnlockRetries *
+mm_iface_modem_get_unlock_retries (MMIfaceModem *self)
+{
+MmGdbusModem *skeleton = NULL;
+MMUnlockRetries *unlock_retries;
+
+g_object_get (self,
+ MM_IFACE_MODEM_DBUS_SKELETON, &skeleton,
+ NULL);
+if (skeleton) {
+GVariant *dictionary;
+
+dictionary = mm_gdbus_modem_get_unlock_retries (skeleton);
+unlock_retries = mm_unlock_retries_new_from_dictionary (dictionary);
+g_object_unref (skeleton);
+} else
+unlock_retries = mm_unlock_retries_new ();
+
+return unlock_retries;
+}
+
void
mm_iface_modem_update_unlock_retries (MMIfaceModem *self,
MMUnlockRetries *unlock_retries)
diff --git a/src/mm-iface-modem.h b/src/mm-iface-modem.h
index af039b3b..1a154b48 100644
--- a/src/mm-iface-modem.h
+++ b/src/mm-iface-modem.h
@@ -414,6 +414,8 @@ MMModemLock mm_iface_modem_update_lock_info_finish
(MMIfaceModem *self,
GAsyncResult *res,
GError **error);
+MMUnlockRetries *mm_iface_modem_get_unlock_retries (MMIfaceModem *self);
+
void mm_iface_modem_update_unlock_retries (MMIfaceModem *self,
MMUnlockRetries *unlock_retries);
--
2.14.1.690.gbb1197296e-goog
___
ModemManager-devel mailing list
ModemManager-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/modemmanager-devel
Re: [review] https://github.com/cbchan/ModemManager/tree/sim-mbim-unlock-retries
> > I overlooked one thing when I revised the patches based on Dan's > suggestion, and also missed that in my testing :-( > > MMSimMbim:update_modem_unlock_retries() calls > mm_iface_modem_update_unlock_retries() to report remaining attempts > PIN1. That won't affect MMBroadbandModemMbimPriv::sim_pin_retries. My > original approach was to handle all MMUnlockRetries updates via > MMIfaceModem. > > If we prefer caching PIN1 retries in MMBroadbandModemMbim, > MMBroadbandModemMbim needs to provide a method (e.g. > mm_broadband_modem_mbm_set_sim_pin_retries) for MMSimMbim to propagate > the information. Would you prefer that or should I go back to my > original approach? > Probably better to go with the old approach I think; I wouldn't like MMIfaceModem code to call MMBroadbandModemMbim APIs. -- Aleksander https://aleksander.es ___ ModemManager-devel mailing list ModemManager-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/modemmanager-devel
Re: [review] https://github.com/cbchan/ModemManager/tree/modem-hardware-revision
Please note that I use GSimpleAsyncResult instead of GTask in the patch that modifies MMBroadbandModemQmi in order to minimize the changes (as I don't want to touch ensure_qmi_client related code). The GTask migration will be done in another branch: https://github.com/cbchan/ModemManager/tree/gtask-broadband-modem-qmi On Mon, Sep 18, 2017 at 11:13 PM, Ben Chan wrote: > Similar to firmware revision, hardware revision is useful information > for identifying characteristics about a modem module. Both MBIM and > QMI provides API to query hardware revision from the modem. This > series of patch add support of loading and reporting hardware revision > through the Modem interface. > > https://github.com/linux-mobile-broadband/ModemManager/compare/master...cbchan:modem-hardware-revision ___ ModemManager-devel mailing list ModemManager-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/modemmanager-devel
[review] https://github.com/cbchan/ModemManager/tree/modem-hardware-revision
Similar to firmware revision, hardware revision is useful information for identifying characteristics about a modem module. Both MBIM and QMI provides API to query hardware revision from the modem. This series of patch add support of loading and reporting hardware revision through the Modem interface. https://github.com/linux-mobile-broadband/ModemManager/compare/master...cbchan:modem-hardware-revision ___ ModemManager-devel mailing list ModemManager-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/modemmanager-devel
Re: [review] https://github.com/cbchan/ModemManager/tree/sim-mbim-unlock-retries
Hi Dan and Aleksander, I overlooked one thing when I revised the patches based on Dan's suggestion, and also missed that in my testing :-( MMSimMbim:update_modem_unlock_retries() calls mm_iface_modem_update_unlock_retries() to report remaining attempts PIN1. That won't affect MMBroadbandModemMbimPriv::sim_pin_retries. My original approach was to handle all MMUnlockRetries updates via MMIfaceModem. If we prefer caching PIN1 retries in MMBroadbandModemMbim, MMBroadbandModemMbim needs to provide a method (e.g. mm_broadband_modem_mbm_set_sim_pin_retries) for MMSimMbim to propagate the information. Would you prefer that or should I go back to my original approach? Thanks, Ben On Fri, Sep 8, 2017 at 1:31 AM, Aleksander Morgado wrote: > On Fri, Aug 11, 2017 at 1:02 AM, Ben Chan wrote: >> On Thu, Aug 10, 2017 at 3:34 PM, Aleksander Morgado >> wrote: >> However, we may be able to improve the situation a bit when MM is >> running by making mm_iface_modem_update_lock_info preserve what we >> have learned from a prior MBIM_CID_PIN response. That's what I'm >> trying to address with this patch set. >> > > Yes, I agree, keeping the information returned in a response is > useful, but why not fully overwrite the list of unlock attempts > instead of merging as you were trying? Let me make sure I understand your proposal. Are you suggesting the following? (1) When MMSimMbim receives a response to a MBIM_CID_PIN set operation, it always overwrite IfaceModem's MMUnlockRetries as MBIM_CID_PIN provides the most relevant info at that point, which I think makes sense (e.g. we really don't care about PIN2 in MM, at least for now. And we don't care about PIN1 when PUK1 is active) (2) When MMBroadbandModemMbim later reload unlock retries via a MBIM_CID_PIN query, we try to preserve PIN1 in IfaceModem's MMUnlockRetries if MBIM_CID_PIN query reports PIN2, for example. >>> >>> I was actually suggesting we leave this as it is now (i.e. query pin >>> lock and retries, and just use that, without merging)... >>> >>> From a user point of view, we have the main features we need: >>> * If locked, we know the lock enabled and the amount of unlock retries >>> left. >>> * We know if a lock unlock operation succeeds or fails, and if it >>> fails we know the amount of unlock retries left. >>> * We know if a lock enable/disable operation succeeds or fails. >>> >>> What we don't know is the amount of retries left if the lock >>> enable/disable operation fails. We could try to hack something in MM >>> to keep some of that logic, but then, if the device/system is rebooted >>> between e.g. failed lock enable attempts, we're back without knowing >>> anything until we let the user retry again. There's no perfect >>> solution either way, not sure. >> >> I totally agree that we don't have a perfect solution here. The >> workaround I propose here is a bit of a hack. The part that I'm not >> very comfortable with the current situation is that an UI can't >> indicate the remaining attempts for PIN1 after a user tries to enable >> the SIM lock with the wrong PIN (even though we've got the infor about >> PIN1, which is overwritten by info about PIN2). >> >> What makes the situation worse from a user perspective is how >> counter-intuitive the SIM lock mechanism is. Some users may actually >> think that they should enter a new PIN when enabling the SIM lock. >> They may not know that a SIM may come with a default PIN. Or they may >> believe that when they previously disabled the SIM lock, the PIN was >> clear. So knowing the remaining attempts for PIN1 when they try to >> enable the SIM lock would be quite helpful as they may stop entering a >> new PIN after the first failed attempt. >> >> The SIM lock mechanism is dated and it seems overlooked that MBIM >> provides no way to explicitly query the remaining attempts for PIN1. >> Unfortunately, we will live with these issues until the SIM locking >> mechanism is deprecated. >> >> Anyway, I'd try one more time :) I've modified the patch to address >> the issues with PIN-PUK transition and tried it on a SIM (finally I >> got the PUK code :p). Here's the mmcli output. Thanks again for >> reviewing. Let me know how you think. >> > > These got merged to git master. > > -- > Aleksander > https://aleksander.es ___ ModemManager-devel mailing list ModemManager-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/modemmanager-devel
