Re: [PATCH 3/3] staging: vme: make match() driver specific toimprove non-VME64x support

[Manohar Vanga]

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/