| Message ID | 547CF9FC.5010101@southpole.se (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
> So at first it would be nice if someone could confirm my findings. > Applying the same kind of code like my patch and unplug something that > uses the affected frontend should be enough. I tried that for tc90522, and I could remove earth-pt3 (which uses tc90522), tc90522 and tuner modules without any problem, although earth-pt3 is a pci driver and does not use dvb-usb-v2. From your log(?) output, I guess that rtl28xxu_exit() removed the attached demod module (mn88472) and thus free'ed fe BEFORE calling dvb_usbv2_exit(), from where dvb_unregister_frontend(fe) is called. I think that the demod i2c device is removed automatically by dvb_usbv2_i2c_exit() in dvb_usbv2_exit(), if you registered the demod i2c device, and your adapter/bridge driver should not try to remove it. regards, Akihiro -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 12/02/2014 11:47 AM, Akihiro TSUKADA wrote: >> So at first it would be nice if someone could confirm my findings. >> Applying the same kind of code like my patch and unplug something that >> uses the affected frontend should be enough. > > I tried that for tc90522, and I could remove earth-pt3 > (which uses tc90522), tc90522 and tuner modules without any problem, > although earth-pt3 is a pci driver and does not use dvb-usb-v2. > >>From your log(?) output, > I guess that rtl28xxu_exit() removed the attached demod module > (mn88472) and thus free'ed fe BEFORE calling dvb_usbv2_exit(), > from where dvb_unregister_frontend(fe) is called. > I think that the demod i2c device is removed automatically by > dvb_usbv2_i2c_exit() in dvb_usbv2_exit(), if you registered > the demod i2c device, and your adapter/bridge driver > should not try to remove it. Yes. You must unregister frontend before you remove driver. I have already added new callbacks detach tuner and frontend to avoid that, but there was yet again new issue as it removes rtl2832 demod driver first and mn88472 slave demod was put to i2c bus / adapter which is owned by rtl2832. So it will crash too. Solution is to convert rtl2832 to I2C binding (or convert mn88472 legacy DVB binding (which I don't allow :)). When rtl2832 driver is converted to I2C model it is not unloaded automatically and you could remove those in a correct order. But hey, mn88472 is still on staging :D regards Antti
On 2014-12-02 11:02, Antti Palosaari wrote: > > > On 12/02/2014 11:47 AM, Akihiro TSUKADA wrote: >>> So at first it would be nice if someone could confirm my findings. >>> Applying the same kind of code like my patch and unplug something that >>> uses the affected frontend should be enough. >> >> I tried that for tc90522, and I could remove earth-pt3 >> (which uses tc90522), tc90522 and tuner modules without any problem, >> although earth-pt3 is a pci driver and does not use dvb-usb-v2. >> >>> From your log(?) output, >> I guess that rtl28xxu_exit() removed the attached demod module >> (mn88472) and thus free'ed fe BEFORE calling dvb_usbv2_exit(), >> from where dvb_unregister_frontend(fe) is called. >> I think that the demod i2c device is removed automatically by >> dvb_usbv2_i2c_exit() in dvb_usbv2_exit(), if you registered >> the demod i2c device, and your adapter/bridge driver >> should not try to remove it. > > Yes. You must unregister frontend before you remove driver. I have > already added new callbacks detach tuner and frontend to avoid that, > but there was yet again new issue as it removes rtl2832 demod driver > first and mn88472 slave demod was put to i2c bus / adapter which is > owned by rtl2832. So it will crash too. Solution is to convert rtl2832 > to I2C binding (or convert mn88472 legacy DVB binding (which I don't > allow :)). When rtl2832 driver is converted to I2C model it is not > unloaded automatically and you could remove those in a correct order. > > But hey, mn88472 is still on staging :D > > regards > Antti > So the solution is to change rtl2832.c to the I2C model? And does this issue only affect the mn8847x drivers ? If this is the case would a patch that does not free the buffer but leaks the memory be ok ? I can add a todo item and log it in syslog. That would for sure be better then crashing the subsystem and the driver is still in staging for a reason. MvH Benjamin Larsson -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 12/02/2014 12:41 PM, Benjamin Larsson wrote: > On 2014-12-02 11:02, Antti Palosaari wrote: >> >> >> On 12/02/2014 11:47 AM, Akihiro TSUKADA wrote: >>>> So at first it would be nice if someone could confirm my findings. >>>> Applying the same kind of code like my patch and unplug something that >>>> uses the affected frontend should be enough. >>> >>> I tried that for tc90522, and I could remove earth-pt3 >>> (which uses tc90522), tc90522 and tuner modules without any problem, >>> although earth-pt3 is a pci driver and does not use dvb-usb-v2. >>> >>>> From your log(?) output, >>> I guess that rtl28xxu_exit() removed the attached demod module >>> (mn88472) and thus free'ed fe BEFORE calling dvb_usbv2_exit(), >>> from where dvb_unregister_frontend(fe) is called. >>> I think that the demod i2c device is removed automatically by >>> dvb_usbv2_i2c_exit() in dvb_usbv2_exit(), if you registered >>> the demod i2c device, and your adapter/bridge driver >>> should not try to remove it. >> >> Yes. You must unregister frontend before you remove driver. I have >> already added new callbacks detach tuner and frontend to avoid that, >> but there was yet again new issue as it removes rtl2832 demod driver >> first and mn88472 slave demod was put to i2c bus / adapter which is >> owned by rtl2832. So it will crash too. Solution is to convert rtl2832 >> to I2C binding (or convert mn88472 legacy DVB binding (which I don't >> allow :)). When rtl2832 driver is converted to I2C model it is not >> unloaded automatically and you could remove those in a correct order. >> >> But hey, mn88472 is still on staging :D >> >> regards >> Antti >> > > So the solution is to change rtl2832.c to the I2C model? And does this > issue only affect the mn8847x drivers ? It likely affects some other dvb-usb-v2 drivers too. But not af9035 as I fixed it initially there I think. > If this is the case would a patch that does not free the buffer but > leaks the memory be ok ? I can add a todo item and log it in syslog. > That would for sure be better then crashing the subsystem and the driver > is still in staging for a reason. Maybe yes, but it does not sound absolute any good. I think you will need to set FE pointer NULL after driver is removed. Then unregister frontend will not call members of that struct anymore, but leak memory? regards Antti
On 2014-12-02 11:59, Antti Palosaari wrote: > [...] >> So the solution is to change rtl2832.c to the I2C model? And does this >> issue only affect the mn8847x drivers ? > > It likely affects some other dvb-usb-v2 drivers too. But not af9035 as > I fixed it initially there I think. > >> If this is the case would a patch that does not free the buffer but >> leaks the memory be ok ? I can add a todo item and log it in syslog. >> That would for sure be better then crashing the subsystem and the driver >> is still in staging for a reason. > > Maybe yes, but it does not sound absolute any good. I think you will > need to set FE pointer NULL after driver is removed. It is NULL now, that is why it is crashing, or the current code leads to random corruptions. > Then unregister frontend will not call members of that struct anymore, > but leak memory? Well any solution that does not randomly crash the kernel when unloading the module is fine by me. My suggestion is to leak the memory and put a note about it in syslog. But I guess there are only a handful of users of this driver so maybe leave it as it is right now? It must be fixed anyway before the driver is moved out of staging. > > regards > Antti > MvH Benjamin Larsson -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/staging/media/mn88472/mn88472.c b/drivers/staging/media/mn88472/mn88472.c index 52de8f8..58af319 100644 --- a/drivers/staging/media/mn88472/mn88472.c +++ b/drivers/staging/media/mn88472/mn88472.c @@ -494,6 +494,7 @@ static int mn88472_remove(struct i2c_client *client) regmap_exit(dev->regmap[0]); + memset(dev, 0, sizeof(*dev)); kfree(dev);