| Message ID | 1444123482-25579-4-git-send-email-architt@codeaurora.org (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Andy Gross |
| Headers | show |
On 10/06/2015 11:24 AM, Archit Taneja wrote: > We don't check whether a previously registered mipi_dsi_device under the > same host shares the same virtual channel. > > Before registering, check if any of the registered devices doesn't > already have the same virtual channel. > > This wasn't crucial when all the devices under a host were populated via > DT. Now that we also support creating devices manually, we could end up > in a situation where a driver tries to create a device with a virtual > channel already taken by a device populated in DT. > > Signed-off-by: Archit Taneja <architt@codeaurora.org> Beside few non-blcking nitpicks. Reviewed-by: Andrzej Hajda <a.hajda@samsung.com> > --- > drivers/gpu/drm/drm_mipi_dsi.c | 26 ++++++++++++++++++++++++-- > 1 file changed, 24 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c > index 46ee515..db6130a 100644 > --- a/drivers/gpu/drm/drm_mipi_dsi.c > +++ b/drivers/gpu/drm/drm_mipi_dsi.c > @@ -123,6 +123,22 @@ static const struct device_type mipi_dsi_device_type = { > .release = mipi_dsi_dev_release, > }; > > +static int __dsi_check_chan_busy(struct device *dev, void *data) > +{ > + struct mipi_dsi_device *dsi = to_mipi_dsi_device(dev); > + u32 reg = *(u32 *) data; > + > + if (dsi && dsi->channel == reg) Maybe simpler would be: u32 *reg = data; if (dsi && dsi->channel == *reg) > + return -EBUSY; > + > + return 0; > +} > + > +static int mipi_dsi_check_chan_busy(struct mipi_dsi_host *host, u32 reg) > +{ > + return device_for_each_child(host->dev, ®, __dsi_check_chan_busy); > +} > + > struct mipi_dsi_device *mipi_dsi_device_new(struct mipi_dsi_host *host, > struct mipi_dsi_device_info *info) > { > @@ -150,14 +166,20 @@ struct mipi_dsi_device *mipi_dsi_device_new(struct mipi_dsi_host *host, > > dev_set_name(&dsi->dev, "%s.%d", dev_name(host->dev), info->reg); > > + r = mipi_dsi_check_chan_busy(host, info->reg); I know this r variable was introduced in the 1st patch, but if you will send next iteration consider replacing it with ret, looks more readable and follows file convention :) Regards Andrzej > + if (r) > + goto err; > + > r = device_register(&dsi->dev); > if (r) { > dev_err(dev, "failed to register device: %d\n", r); > - kfree(dsi); > - return ERR_PTR(r); > + goto err; > } > > return dsi; > +err: > + kfree(dsi); > + return ERR_PTR(r); > } > EXPORT_SYMBOL(mipi_dsi_device_new); > -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10/30/2015 06:22 PM, Andrzej Hajda wrote: > On 10/06/2015 11:24 AM, Archit Taneja wrote: >> We don't check whether a previously registered mipi_dsi_device under the >> same host shares the same virtual channel. >> >> Before registering, check if any of the registered devices doesn't >> already have the same virtual channel. >> >> This wasn't crucial when all the devices under a host were populated via >> DT. Now that we also support creating devices manually, we could end up >> in a situation where a driver tries to create a device with a virtual >> channel already taken by a device populated in DT. >> >> Signed-off-by: Archit Taneja <architt@codeaurora.org> > Beside few non-blcking nitpicks. > > Reviewed-by: Andrzej Hajda <a.hajda@samsung.com> > >> --- >> drivers/gpu/drm/drm_mipi_dsi.c | 26 ++++++++++++++++++++++++-- >> 1 file changed, 24 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c >> index 46ee515..db6130a 100644 >> --- a/drivers/gpu/drm/drm_mipi_dsi.c >> +++ b/drivers/gpu/drm/drm_mipi_dsi.c >> @@ -123,6 +123,22 @@ static const struct device_type mipi_dsi_device_type = { >> .release = mipi_dsi_dev_release, >> }; >> >> +static int __dsi_check_chan_busy(struct device *dev, void *data) >> +{ >> + struct mipi_dsi_device *dsi = to_mipi_dsi_device(dev); >> + u32 reg = *(u32 *) data; >> + >> + if (dsi && dsi->channel == reg) > Maybe simpler would be: > > u32 *reg = data; > > if (dsi && dsi->channel == *reg) > This is better. I'll change it to this. > > >> + return -EBUSY; >> + >> + return 0; >> +} >> + >> +static int mipi_dsi_check_chan_busy(struct mipi_dsi_host *host, u32 reg) >> +{ >> + return device_for_each_child(host->dev, ®, __dsi_check_chan_busy); >> +} >> + >> struct mipi_dsi_device *mipi_dsi_device_new(struct mipi_dsi_host *host, >> struct mipi_dsi_device_info *info) >> { >> @@ -150,14 +166,20 @@ struct mipi_dsi_device *mipi_dsi_device_new(struct mipi_dsi_host *host, >> >> dev_set_name(&dsi->dev, "%s.%d", dev_name(host->dev), info->reg); >> >> + r = mipi_dsi_check_chan_busy(host, info->reg); > > I know this r variable was introduced in the 1st patch, but if you will send > next iteration > consider replacing it with ret, looks more readable and follows file convention :) I'll probably end up sending another iteration (or more). I'll change the name. Thanks, Archit > > Regards > Andrzej > >> + if (r) >> + goto err; >> + >> r = device_register(&dsi->dev); >> if (r) { >> dev_err(dev, "failed to register device: %d\n", r); >> - kfree(dsi); >> - return ERR_PTR(r); >> + goto err; >> } >> >> return dsi; >> +err: >> + kfree(dsi); >> + return ERR_PTR(r); >> } >> EXPORT_SYMBOL(mipi_dsi_device_new); >> > > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ >
diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c index 46ee515..db6130a 100644 --- a/drivers/gpu/drm/drm_mipi_dsi.c +++ b/drivers/gpu/drm/drm_mipi_dsi.c @@ -123,6 +123,22 @@ static const struct device_type mipi_dsi_device_type = { .release = mipi_dsi_dev_release, }; +static int __dsi_check_chan_busy(struct device *dev, void *data) +{ + struct mipi_dsi_device *dsi = to_mipi_dsi_device(dev); + u32 reg = *(u32 *) data; + + if (dsi && dsi->channel == reg) + return -EBUSY; + + return 0; +} + +static int mipi_dsi_check_chan_busy(struct mipi_dsi_host *host, u32 reg) +{ + return device_for_each_child(host->dev, ®, __dsi_check_chan_busy); +} + struct mipi_dsi_device *mipi_dsi_device_new(struct mipi_dsi_host *host, struct mipi_dsi_device_info *info) { @@ -150,14 +166,20 @@ struct mipi_dsi_device *mipi_dsi_device_new(struct mipi_dsi_host *host, dev_set_name(&dsi->dev, "%s.%d", dev_name(host->dev), info->reg); + r = mipi_dsi_check_chan_busy(host, info->reg); + if (r) + goto err; + r = device_register(&dsi->dev); if (r) { dev_err(dev, "failed to register device: %d\n", r); - kfree(dsi); - return ERR_PTR(r); + goto err; } return dsi; +err: + kfree(dsi); + return ERR_PTR(r); } EXPORT_SYMBOL(mipi_dsi_device_new);
We don't check whether a previously registered mipi_dsi_device under the same host shares the same virtual channel. Before registering, check if any of the registered devices doesn't already have the same virtual channel. This wasn't crucial when all the devices under a host were populated via DT. Now that we also support creating devices manually, we could end up in a situation where a driver tries to create a device with a virtual channel already taken by a device populated in DT. Signed-off-by: Archit Taneja <architt@codeaurora.org> --- drivers/gpu/drm/drm_mipi_dsi.c | 26 ++++++++++++++++++++++++-- 1 file changed, 24 insertions(+), 2 deletions(-)