Hey Emilio,
On Wed, Aug 31, 2011 at 05:20:47PM -0400, Emilio G. Cota wrote:
> This was hard to review. There are references to functions that
> are not committed in Greg's tree yet ("staging" tree @ git.kernel.org).
>
> I assume this patch was applied before you wrote the v4 patchset:
>
> https://lkml.org/lkml/2011/8/12/107
>
I believe Greg has acked this patch (I received a confirmation mail from him).
> On Wed, Aug 31, 2011 at 12:05:46 +0200, Manohar Vanga wrote:
> (snip)
> > Another change introduced in this patch is that devices are now created
> > within the VME driver structure rather than in the VME bridge structure.
> > This way, things don't go haywire if the bridge driver is removed while
> > a driver is using it (this is also additionally prevented by having
> > reference counting of used bridge modules).
>
> The mention to refcounting seems outdated. As I stated in my reply
> to v0, we should just safely remove devices under the bus when
> vme_unregister_bus() is called.
Ah right need to reword that.
> > -void vme_unregister_bridge(struct vme_bridge *bridge)
> > {
> > - int i;
> > - struct vme_dev *vdev;
> > -
> > -
> > - for (i = 0; i < VME_SLOTS_MAX; i++) {
> > - vdev = bridge->dev[i];
> > - device_unregister(&vdev->dev);
> > - }
> > vme_remove_bus(bridge);
> > }
>
> So we're essentially leaving the devices there, even though the
> bridge they're under will be removed. This doesn't seem right.
> btw with the removal of the array of vme_dev's from struct vme_bridge,
> the bridge cannot know which devices are under it.
>
> We have to bear in mind that the drv->devices list needs to be
> updated when devices come and go; possibly a bridge->devices list
> could also be kept.
>
> Helpers around device_register and _unregister may simplify the lists'
> housekeeping.
I was going to add a separate patch for this but I'll just integrate into this
one (makes more sense anyway).
And yes, I also noticed that the bridge no longer has track of its devices and
bridges will need to keep a list of them.
> > - return retval;
> > + if (vdev->dev.platform_data) {
> > + list_add_tail(&vdev->list, &drv->devices);
> > + drv->ndev++;
>
> Ok, so drv->ndev can only increase. In case a device is removed (when
> a bus driver is removed) this may need to be decreased, which isn't
> done in the corresponding list_del() calls (I've marked them).
>
> In fact I wonder whether it is useful at all to have drv->ndev. What's
> its purpose?
I'm not sure why I added that now...
It can be removed.
--
/manohar
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/devel
