| Message ID | 20190717141204.19433-1-bnvandana@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media:dvb-frontends:Return if Max devices are added in dvb_pll_attach(). | expand |
Hi, > diff --git a/drivers/media/dvb-frontends/dvb-pll.c b/drivers/media/dvb-frontends/dvb-pll.c > index ba0c49107bd2..c850f1d69bce 100644 > --- a/drivers/media/dvb-frontends/dvb-pll.c > +++ b/drivers/media/dvb-frontends/dvb-pll.c > @@ -788,6 +788,9 @@ struct dvb_frontend *dvb_pll_attach(struct dvb_frontend *fe, int pll_addr, > int ret; > const struct dvb_pll_desc *desc; > > + if (dvb_pll_devcount > DVB_PLL_MAX - 1) > + return NULL; > + > b1 = kmalloc(1, GFP_KERNEL); > if (!b1) > return NULL; > Wouldn't it put a limit on the number of attachment of devices? I'm afraid that an user may repeatedly plugs in and off a device using this driver (for some reason), and finally gets an error. Since dvb_pll_devcount and "id" module parameter are just used for debugging purpose to override/force PLL type, removing them totally would be better, IMHO. regards, Akihiro
On 18/07/19 6:05 AM, Akihiro TSUKADA wrote: > Hi, > >> diff --git a/drivers/media/dvb-frontends/dvb-pll.c b/drivers/media/dvb-frontends/dvb-pll.c >> index ba0c49107bd2..c850f1d69bce 100644 >> --- a/drivers/media/dvb-frontends/dvb-pll.c >> +++ b/drivers/media/dvb-frontends/dvb-pll.c >> @@ -788,6 +788,9 @@ struct dvb_frontend *dvb_pll_attach(struct dvb_frontend *fe, int pll_addr, >> int ret; >> const struct dvb_pll_desc *desc; >> >> + if (dvb_pll_devcount > DVB_PLL_MAX - 1) >> + return NULL; >> + >> b1 = kmalloc(1, GFP_KERNEL); >> if (!b1) >> return NULL; >> > Wouldn't it put a limit on the number of attachment of devices? > I'm afraid that an user may repeatedly plugs in and off a device > using this driver (for some reason), and finally gets an error. > > Since dvb_pll_devcount and "id" module parameter are just used > for debugging purpose to override/force PLL type, > removing them totally would be better, IMHO. Hi, Thanks for reviewing the patch. Will it be better, if dvb_pll_devcount is decremented in dvb_pll_release(), instead of removing module params? Regards, Vandana. > > regards, > Akihiro
> Will it be better, if dvb_pll_devcount is decremented in dvb_pll_release(), instead of removing module params?
But you cannot know deterministically which device corrensponds to
what "id" (when you have multiple dvb_pll devices),
since "id" is dependent on the history of register and unregister
of dvb_pll devices.
So I wonder about the benefit / practical usecase of "id" parameter.
--
Akihiro
On 19/07/19 7:11 AM, Akihiro TSUKADA wrote: >> Will it be better, if dvb_pll_devcount is decremented in dvb_pll_release(), instead of removing module params? > But you cannot know deterministically which device corrensponds to > what "id" (when you have multiple dvb_pll devices), > since "id" is dependent on the history of register and unregister > of dvb_pll devices. dvb_pll_release() frees fe->tuner_priv, and priv->nr is dvb_pll_devcount, but, decrementing count will only tell array has a free slot, and now if that free slot needs to be used it will have to either maintain free index list to be consumed next or convert array id to a list. > So I wonder about the benefit / practical usecase of "id" parameter. Ok, I will remove the module parameters and send a patch. Thanks, Vandana. > > -- > Akihiro
diff --git a/drivers/media/dvb-frontends/dvb-pll.c b/drivers/media/dvb-frontends/dvb-pll.c index ba0c49107bd2..c850f1d69bce 100644 --- a/drivers/media/dvb-frontends/dvb-pll.c +++ b/drivers/media/dvb-frontends/dvb-pll.c @@ -788,6 +788,9 @@ struct dvb_frontend *dvb_pll_attach(struct dvb_frontend *fe, int pll_addr, int ret; const struct dvb_pll_desc *desc; + if (dvb_pll_devcount > DVB_PLL_MAX - 1) + return NULL; + b1 = kmalloc(1, GFP_KERNEL); if (!b1) return NULL;