Hi Denis, I took me a while to get back to this, but we need to work it out...
On 12 January 2011 22:55, Denis Kenzior <[email protected]> wrote: >> +void __ofono_sim_refresh(struct ofono_sim *sim, GSList *file_list, >> + ofono_bool_t full_file_change, ofono_bool_t naa_init) >> +{ >> + GSList *l, *filel; >> + enum sim_reset_state { >> + RESET_STATE_NOT_PRESENT = OFONO_SIM_STATE_NOT_PRESENT, >> + RESET_STATE_INSERTED = OFONO_SIM_STATE_INSERTED, >> + RESET_STATE_READY = OFONO_SIM_STATE_READY, >> + RESET_STATE_NONE, >> + } reset_state = RESET_STATE_NONE; >> + >> + /* Flush cached content for affected files */ >> + if (full_file_change) >> + sim_fs_cache_flush(sim->simfs); >> + else >> + for (filel = file_list; filel; filel = filel->next) { >> + struct stk_file *file = filel->data; >> + int id = (file->file[file->len - 2] << 8) | >> + (file->file[file->len - 1] << 0); >> + >> + sim_file_changed_flush(sim, id); >> + } > > Just a nitpick, but when you have such compound statements inside > if/while/for, could you enclose them in parentheses? I think that makes > the code a bit more readable. > > e.g. > if (foo) > else { > for () { > > } > } Ok, good point. > >> + >> + if (naa_init) >> + reset_state = RESET_STATE_INSERTED; >> + >> + /* >> + * Check if we have file change handlers for all of the affected >> + * files. If not, we will fall back to re-initialising the >> + * application which ensures that all files are re-read. >> + */ >> + for (l = sim->fs_watches; l; l = l->next) { >> + struct fs_watch *w = l->data; >> + >> + if (full_file_change) { >> + if (w->notify) >> + continue; >> + >> + if (w->reset_state < reset_state) >> + reset_state = reset_state; >> + >> + continue; >> + } >> + >> + for (filel = file_list; filel; filel = filel->next) { >> + struct stk_file *file = filel->data; >> + int id = (file->file[file->len - 2] << 8) | >> + (file->file[file->len - 1] << 0); >> + >> + if (id != w->id) >> + continue; >> + >> + if (w->notify) >> + break; >> + >> + if (w->reset_state < reset_state) >> + reset_state = reset_state; >> + >> + break; >> + } >> + } >> + >> + /* >> + * Notify the subscribers of files that have changed unless we >> + * have determined that a re-initialisation is necessary and >> + * will trigger re-reading of those files anyway. >> + */ >> + for (l = sim->fs_watches; l; l = l->next) { >> + struct fs_watch *w = l->data; >> + >> + if (full_file_change) { >> + if (w->reset_state < reset_state) >> + w->notify(w->id, w->notify_data); >> + >> + continue; >> + } >> + >> + for (filel = file_list; filel; filel = filel->next) { >> + struct stk_file *file = filel->data; >> + int id = (file->file[file->len - 2] << 8) | >> + (file->file[file->len - 1] << 0); >> + >> + if (id != w->id) >> + continue; >> + >> + if (w->reset_state < reset_state) >> + w->notify(w->id, w->notify_data); >> + >> + break; >> + } >> + } >> + >> + switch (reset_state) { >> + case RESET_STATE_NOT_PRESENT: >> + ofono_sim_inserted_notify(sim, FALSE); >> + ofono_sim_inserted_notify(sim, TRUE); > > Are you sure we can do it this way? I would assume that the modem might > require some time to perform its own SIM initialization and would have > to signal the sim_inserted itself... I assume that the modem would send the refresh command only once it has done all its internal housekeeping and is ready for us to re-read the changed files. If not then probably the AT interface will block for as much time as needed? > >> + break; >> + case RESET_STATE_INSERTED: >> + sim_free_state(sim); >> + sim->state = OFONO_SIM_STATE_INSERTED; >> + sim_initialize(sim); > > What about removing all the post_sim / online atoms? In theory > something like EFfdn could have been changed and now we're stuck in > fixed dialing mode. We should not keep the atoms around in this case. So in the next patch I add a "watch" for every file in the SIM that we care about. Every watch has a sort of "reset level", which tells sim.c how much of our state needs to be reinitialised, so that we don't reset the atoms if that is not necessary. For example for EFust / EFest we set the level to RESET_STATE_INSERTED. This is the case for FDN, because will be re-read EFust/EFest as a result of sim_initialize() and later at some point we will signal OFONO_SIM_STATE_READY so all the concerned atoms will take note. It's not necessary to remove the atoms in this case. > >> + break; >> + case RESET_STATE_READY: >> + sim->state = OFONO_SIM_STATE_INSERTED; >> + for (l = sim->state_watches->items; l; l = l->next) { >> + struct ofono_watchlist_item *item = l->data; >> + ofono_sim_state_event_cb_t notify = item->notify; >> + >> + notify(sim->state, item->notify_data); > > I'm having trouble understanding the need for this for loop. Isn't the > SIM already inserted? I initially didn't have this loop, but I thought it might be better to signal "INSERTED" just before "READY", so that atoms see a *change* if they though that the sim was READY before. But looking at the current users of the sim state watch, it makes no difference. > >> + } >> + >> + sim_set_ready(sim); >> + break; >> + case RESET_STATE_NONE: >> + break; >> + } >> +} > > Overall it seems like there are two categories of files that we should > worry about: > > - Those handled inside sim.c that affect sim initialization: > - EFust, EFest, EFsst > - EFbdn, EFfdn > - EFphase, EFcphs_info (these ones are really unlikely outside of a > full sim reset) > - EFad (This one is unlikely as it would affect the IMSI) > - The ones that don't > > To me it sounds like we should just special case the ones affecting sim > initialization inside this function and use a simplified API for the rest. So I think this wouldn't make the api much better because: * These files you listed are the majority of all files we read, so the "normal" users are in minority. * The only simplification I see would be removal of one parameter in add_file_watch_call int ofono_sim_add_file_watch(struct ofono_sim *sim, int id, enum ofono_sim_state reset_state, ofono_sim_file_notify_t notify, void *userdata); or are you thinking about a different simplifaction? Best regards _______________________________________________ ofono mailing list [email protected] http://lists.ofono.org/listinfo/ofono
