[PATCH] sim: Check SIM pin status after changing pin.

[PATCH] sim: Check SIM pin status after changing pin.

[Marit Henriksen]

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

From: Marit Henriksen <marit.henriksen@stericsson.com>

When changing pin, it is possible to get in a state where the modem requests puk 
(if incorrect pin is entered too many times). Need to check the SIM pin status 
to discover this.
---
 src/sim.c |   15 ++++++++-------
 1 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/src/sim.c b/src/sim.c
index 699ebe9..b2277c5 100644
--- a/src/sim.c
+++ b/src/sim.c
@@ -628,15 +628,16 @@ static DBusMessage *sim_unlock_pin(DBusConnection *conn, DBusMessage *msg,
 static void sim_change_pin_cb(const struct ofono_error *error, void *data)
 {
 	struct ofono_sim *sim = data;
+	DBusMessage *reply;
 
-	if (error->type != OFONO_ERROR_TYPE_NO_ERROR) {
-		__ofono_dbus_pending_reply(&sim->pending,
-				__ofono_error_failed(sim->pending));
-		return;
-	}
+	if (error->type != OFONO_ERROR_TYPE_NO_ERROR)
+		reply = __ofono_error_failed(sim->pending);
+	else
+		reply = dbus_message_new_method_return(sim->pending);
 
-	__ofono_dbus_pending_reply(&sim->pending,
-				dbus_message_new_method_return(sim->pending));
+	__ofono_dbus_pending_reply(&sim->pending, reply);
+
+	sim_pin_check(sim);
 }
 
 static DBusMessage *sim_change_pin(DBusConnection *conn, DBusMessage *msg,
-- 
1.7.0.4

Re: [PATCH] sim: Check SIM pin status after changing pin.

[Denis Kenzior]

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

Hi Marit,

On 11/02/2010 10:08 AM, Marit Henriksen wrote:
> From: Marit Henriksen <marit.henriksen@stericsson.com>
> 
> When changing pin, it is possible to get in a state where the modem requests puk 
> (if incorrect pin is entered too many times). Need to check the SIM pin status 
> to discover this.
> ---
>  src/sim.c |   15 ++++++++-------
>  1 files changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/src/sim.c b/src/sim.c
> index 699ebe9..b2277c5 100644
> --- a/src/sim.c
> +++ b/src/sim.c
> @@ -628,15 +628,16 @@ static DBusMessage *sim_unlock_pin(DBusConnection *conn, DBusMessage *msg,
>  static void sim_change_pin_cb(const struct ofono_error *error, void *data)
>  {
>  	struct ofono_sim *sim = data;
> +	DBusMessage *reply;
>  
> -	if (error->type != OFONO_ERROR_TYPE_NO_ERROR) {
> -		__ofono_dbus_pending_reply(&sim->pending,
> -				__ofono_error_failed(sim->pending));
> -		return;
> -	}
> +	if (error->type != OFONO_ERROR_TYPE_NO_ERROR)
> +		reply = __ofono_error_failed(sim->pending);
> +	else
> +		reply = dbus_message_new_method_return(sim->pending);
>  
> -	__ofono_dbus_pending_reply(&sim->pending,
> -				dbus_message_new_method_return(sim->pending));
> +	__ofono_dbus_pending_reply(&sim->pending, reply);
> +
> +	sim_pin_check(sim);

I don't think that running sim_pin_check is such a good idea.  That
function initializes the sim interface when +CPIN returns READY.

You also need to tear down the modem state back to pre-sim if the modem
asks for a PUK after change pin fails.  That entails removing all
post_sim/post_online atoms and bring the modem back to the offline state ;)

>  }
>  
>  static DBusMessage *sim_change_pin(DBusConnection *conn, DBusMessage *msg,

Regards,
-Denis

Re: [PATCH] sim: Check SIM pin status after changing pin.

[Marit Sofie Henriksen]

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

Hi Denis.
OK, I can see that this approach was rather too simplistic... Maybe checking
the returned CME error would be a better idea?
I’m a bit unsure how (or if) to proceed with this. As this is in the core,
and not our driver, it might be more on your table? If you think it is
important.



Br Marit




2010/11/2 Denis Kenzior <denkenz@gmail.com>

> Hi Marit,
>
> On 11/02/2010 10:08 AM, Marit Henriksen wrote:
> > From: Marit Henriksen <marit.henriksen@stericsson.com>
> >
> > When changing pin, it is possible to get in a state where the modem
> requests puk
> > (if incorrect pin is entered too many times). Need to check the SIM pin
> status
> > to discover this.
> > ---
> >  src/sim.c |   15 ++++++++-------
> >  1 files changed, 8 insertions(+), 7 deletions(-)
> >
> > diff --git a/src/sim.c b/src/sim.c
> > index 699ebe9..b2277c5 100644
> > --- a/src/sim.c
> > +++ b/src/sim.c
> > @@ -628,15 +628,16 @@ static DBusMessage *sim_unlock_pin(DBusConnection
> *conn, DBusMessage *msg,
> >  static void sim_change_pin_cb(const struct ofono_error *error, void
> *data)
> >  {
> >       struct ofono_sim *sim = data;
> > +     DBusMessage *reply;
> >
> > -     if (error->type != OFONO_ERROR_TYPE_NO_ERROR) {
> > -             __ofono_dbus_pending_reply(&sim->pending,
> > -                             __ofono_error_failed(sim->pending));
> > -             return;
> > -     }
> > +     if (error->type != OFONO_ERROR_TYPE_NO_ERROR)
> > +             reply = __ofono_error_failed(sim->pending);
> > +     else
> > +             reply = dbus_message_new_method_return(sim->pending);
> >
> > -     __ofono_dbus_pending_reply(&sim->pending,
> > -
> dbus_message_new_method_return(sim->pending));
> > +     __ofono_dbus_pending_reply(&sim->pending, reply);
> > +
> > +     sim_pin_check(sim);
>
> I don't think that running sim_pin_check is such a good idea.  That
> function initializes the sim interface when +CPIN returns READY.
>
> You also need to tear down the modem state back to pre-sim if the modem
> asks for a PUK after change pin fails.  That entails removing all
> post_sim/post_online atoms and bring the modem back to the offline state ;)
>
> >  }
> >
> >  static DBusMessage *sim_change_pin(DBusConnection *conn, DBusMessage
> *msg,
>
> Regards,
> -Denis
>

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

Re: [PATCH] sim: Check SIM pin status after changing pin.

[Denis Kenzior]

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

Hi Marit,

Just a gentle reminder not to top-post on this mailing list.

On 11/05/2010 09:25 AM, Marit Sofie Henriksen wrote:
> Hi Denis.
> OK, I can see that this approach was rather too simplistic... Maybe
> checking the returned CME error would be a better idea?
> I’m a bit unsure how (or if) to proceed with this. As this is in the
> core, and not our driver, it might be more on your table? If you think
> it is important.
> 

Well this is definitely something we need to get right.  So feel free to
send a patch to the TODO file adding this task.  The description is
basically what I outlined in my earlier reply.  E.g. if change_pin
fails, we should check PIN state.  If it is anything other than READY,
tear the state back to pre_sim state and set the modem offline.

this task can go through the regular task lifecycle.  If you guys feel
up to it, this is certainly something you can work on / contribute.

Regards,
-Denis

Re: [PATCH] sim: Check SIM pin status after changing pin.

[Marcel Holtmann]

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

Hi Marit,

as Denis mentioned and I stated many times, please don't top post on
this mailing list. It is against our mailing list etiquette.

> OK, I can see that this approach was rather too simplistic... Maybe checking
> the returned CME error would be a better idea?
> I’m a bit unsure how (or if) to proceed with this. As this is in the core,
> and not our driver, it might be more on your table? If you think it is
> important.

So if I follow this logic, then it does mean we should not be fixing
issues with the STE driver anymore? And not allowing you to use the
generic AT modem and generic MBM modem drivers? Since these are strictly
speaking not STE driver either.

This is not how this works. Everybody works for the greater good to
improve oFono. This is the way how it makes progress. The core is not a
fixed piece of software. It evolves with the modems we support. If it
needs to be changed or improved, then that is a task you should take up.

And as explained by Denis, if you have found such a problem area inside
the core, the generic drivers or anywhere else, write it down as part of
the TODO list. So patches are highly encouraged.

Regards

Marcel