On Friday 15 July 2011 02:47:27 Mauro Carvalho Chehab wrote: > Em 14-07-2011 20:45, Oliver Endriss escreveu: > > On Monday 04 July 2011 02:17:52 Mauro Carvalho Chehab wrote: > >> Em 03-07-2011 20:24, Oliver Endriss escreveu: > > ... > >>> Anyway, I spent the whole weekend to re-format the code carefully > >>> and create both patch series, trying not to break anything. > >>> I simply cannot go through the driver code and verify everything. > >> > >> As the changes on CHK_ERROR were done via script, it is unlikely that it > >> introduced any problems (well, except if some function is returning > >> a positive value as an error code, but I think that this is not the > >> case). > >> > >> I did the same replacement when I've cleanup the drx-d driver (well, the > >> script were not the same, but it used a similar approach), and the changes > >> didn't break anything, but it is safer to have a test, to be sure that no > >> functional changes were introduced. > >> > >> A simple test with the code and some working board is probably enough > >> to verify that nothing broke. > > > > Finally I found some time to do this 'simple' test. > > Thanks for testing it. Big changes on complex driver require testing. > > > Congratulations! You completely broke the DRXK for ngene and ddbridge: > > - DVB-T tuning does not work anymore. > > I don't have any DVB-T signal here. I'll double check what changed there > and see if I can identify a possible cause for it, but eventually I may > not discover what's wrong. > > Before I start bisecting, I need to know if the starting point is working. > So, had you test that DVB-T was working after your cleanup patches? Yes, it worked. And now I double checked with media_build of July 3th + my patch series: It works as expected. Well, I did not test DVB-C, but people reported that DVB-C was working before I applied my cleanups. So I assume it worked. > > - Module unloading fails as well. drxk is 'in use' due to bad reference count. > > I was eventually expecting a feedback about that. The patch that changed the > behavior is this one: > > http://git.linuxtv.org/media_tree.git?a=commitdiff;h=f087cdd6f4fa8bef0e7588b124f52649d3cebe67 > > What happens is that drxk_attach() allocates one state struct each time it is called, > and it initializes both frontends with the state: drxk_attach() was meant to be called once. See dvb_input_attach() in ddbridge-core.c: case DDB_TUNER_DVBCT_TR: if (demod_attach_drxk(input) < 0) return -ENODEV; if (tuner_attach_tda18271(input) < 0) return -ENODEV; if (input->fe) { if (dvb_register_frontend(adap, input->fe) < 0) return -ENODEV; } if (input->fe2) { if (dvb_register_frontend(adap, input->fe2) < 0) return -ENODEV; input->fe2->tuner_priv=input->fe->tuner_priv; memcpy(&input->fe2->ops.tuner_ops, &input->fe->ops.tuner_ops, sizeof(struct dvb_tuner_ops)); } break; demod_attach_drxk() returns two frontends: fe and fe2. > state = kzalloc(sizeof(struct drxk_state), GFP_KERNEL); > ... > state->c_frontend.demodulator_priv = state; > state->t_frontend.demodulator_priv = state; > ... > *fe_t = &state->t_frontend; > ... > return &state->c_frontend; > > If you call it twice, the driver will loose the references for the first struct, and you'll > end by having a memory leak at driver removal, if both DVB-T and DVB-C are registered. See above. > On my code, I've made sure to call it only once, and then I ended by having a reference = (u32) -1 > for device removal, preventing me to remove the driver, as . > > So, clearly, that function should be called just once. However, at DVB unregister, > dvb_frontend_detach() will be called twice: > > static void unregister_dvb(struct em28xx_dvb *dvb) > { > dvb_net_release(&dvb->net); > dvb->demux.dmx.remove_frontend(&dvb->demux.dmx, &dvb->fe_mem); > dvb->demux.dmx.remove_frontend(&dvb->demux.dmx, &dvb->fe_hw); > dvb_dmxdev_release(&dvb->dmxdev); > dvb_dmx_release(&dvb->demux); > if (dvb->fe[1]) > dvb_unregister_frontend(dvb->fe[1]); > dvb_unregister_frontend(dvb->fe[0]); > if (dvb->fe[1]) > dvb_frontend_detach(dvb->fe[1]); > dvb_frontend_detach(dvb->fe[0]); > dvb_unregister_adapter(&dvb->adapter); > } >From ddbridge-core.c: if (input->fe2) dvb_unregister_frontend(input->fe2); if (input->fe) { dvb_unregister_frontend(input->fe); dvb_frontend_detach(input->fe); input->fe = NULL; } So dvb_frontend_detach() was called exactly once. Mauro, you must not change the logic of the frontend driver, unless you verified what the bridge driver does... > Or we would need to write a special unregister function at the driver just to > cover the cases where DRX-K is used, as, on all other drivers, the attach > function is called twice, even when the frontend supports two different types. > > See cxd2820r_attach, for example: instead of initializing both frontends > with just one call, it can be called twice. If called a second time, it will > just use the previously allocated data. > > I think that the better is to revert my patch and apply a solution similar > to cxd2820r_attach. It should work fine if called just once (like ngene/ddbridge) > or twice (like em28xx). > > > (DVB-C not tested: I currently do not have access to a DVB-C signal.) > > Hmm... are you sure that DVB-C used to work? I found an error on DVB-C setup for > the device I used for test, fixed on this patch: > > http://git.linuxtv.org/media_tree.git?a=commitdiff;h=21ff98772327ff182f54d2fcca69448e440e23d3 > > Basically, on the device I tested, scu command: > SCU_RAM_COMMAND_STANDARD_QAM | SCU_RAM_COMMAND_CMD_DEMOD_SET_PARAM > requires 2 parameters, instead of 4. > > I've preserved the old behavior there, assuming that your code was working, but I suspect that > at least you need to do this: > > + setParamParameters[0] = QAM_TOP_ANNEX_A; > + if (state->m_OperationMode == OM_QAM_ITU_C) > + setEnvParameters[0] = QAM_TOP_ANNEX_C; /* Annex */ > + else > + setEnvParameters[0] = 0; > + > + status = scu_command(state, SCU_RAM_COMMAND_STANDARD_QAM | SCU_RAM_COMMAND_CMD_DEMOD_SET_ENV, 1, setEnvParameters, 1, &cmdResult); > > Due to this logic there, at SetQAM: > > /* Env parameters */ > setEnvParameters[2] = QAM_TOP_ANNEX_A; /* Annex */ > if (state->m_OperationMode == OM_QAM_ITU_C) > setEnvParameters[2] = QAM_TOP_ANNEX_C; /* Annex */ > > This var is filled, but there's no call to SCU_RAM_COMMAND_CMD_DEMOD_SET_ENV. Also, > iti initializes it as parameters[2], instead of parameters[0]. Sorry, I can't test it. Maybe Ralph can comment on this. CU Oliver -- ---------------------------------------------------------------- VDR Remote Plugin 0.4.0: http://www.escape-edv.de/endriss/vdr/ 4 MByte Mod: http://www.escape-edv.de/endriss/dvb-mem-mod/ Full-TS Mod: http://www.escape-edv.de/endriss/dvb-full-ts-mod/ ---------------------------------------------------------------- -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
