lkml.org 
[lkml]   [2011]   [Sep]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 3/3] staging: vme: make match() driver specific to improve non-VME64x support
    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


    \
     
     \ /
      Last update: 2011-09-01 09:51    [W:0.028 / U:31.776 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site