lkml.org 
[lkml]   [2011]   [Mar]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRE: [PATCH] [SCSI] bnx2fc: fix build error when !CONFIG_MODULES
    From
    Date
    On Tue, 2011-03-08 at 11:36 -0800, Zou, Yi wrote: 
    > > On Mon, 2011-03-07 at 16:18 -0800, James Bottomley wrote:
    > > > On Mon, 2011-03-07 at 15:54 -0800, Bhanu Gollapudi wrote:
    > > > > On Mon, 2011-03-07 at 12:16 -0800, Mariusz Kozlowski wrote:
    > > > > > On Wed, Mar 02, 2011 at 11:10:03PM +0100, Mariusz Kozlowski wrote:
    > > > > > > drivers/scsi/bnx2fc/bnx2fc_fcoe.c:1815: error: dereferencing
    > > pointer to incomplete type
    > > > > > > drivers/scsi/bnx2fc/bnx2fc_fcoe.c:1815: error:
    > > ‘MODULE_STATE_LIVE’ undeclared (first use in this function)
    > > > > >
    > > > > > Hm. Still there in next-20110307. Is this patch wrong or..?
    > > > > >
    > > > >
    > > > > James,
    > > > >
    > > > > Here is my ack for this patch.
    > > >
    > > > OK, so the patch is actually wrong because adding #ifdefs on modules in
    > > > files really impedes readability. The bug is using a direct deref on
    > > > module state instead of one of the APIs which work in the non-modular
    > > > case, namely try_module_get(). That means the other two need to come
    > > out
    > > > and be reworked (plus all the others in fcoe).
    > > >
    > > > Reworked looks like it might be a bigger item than bnx2fc. If any of
    > > > those tests is ever relevant, it means we have a race in the
    > > > fcoe_transport because it shouldn't be calling function pointers on a
    > > > dying module (unless it wants to trigger an oops).
    > > >
    > > > So, why are you trying to do this in the first place?
    > > >
    > > First, fcoe.c started with these checks. Here is a comment in fcoe.c at
    > > the point of one of the checks.
    > >
    > > /*
    > > * Make sure the module has been initialized, and is not about to be
    > > * removed. Module paramter sysfs files are writable before the
    > > * module_init function is called and after module_exit.
    > > */
    > >
    > > I don't know the correct way to fix that race is, but we may be past the
    > > need to fix it in the LLDs.
    > >
    > > Next, the fcoe transport was added. Since it (libfcoe.ko) is now calling
    > > what used to be the fcoe.ko sysfs entry points I don't think the problem
    > > exists in fcoe.c or in bnx2fc_fcoe.c, the problem should be in the fcoe
    > > transport code, as James suggested.
    > >
    > > The fcoe transport code already has these checks to protect against
    > > sysfs files being writable before module initialization is complete. It
    > > uses the ft_mutex to protect the list of transports(LLDs) so when
    > > 'create' is called it knows that the transport is still there to call
    > > down to. It holds the ft_mutex until the LLD's 'create' routine returns.
    > > The transports(LLDs) should be detaching themselves from the fcoe
    > > transport layer before they exit. fcoe_transport_detach will try to
    > > acquire the ft_mutex and block until the 'create' call returns and
    > > releases the ft_mutex. I think this ensures that the transport(LLD) will
    > > be fine when the fcoe transport calls it.
    > >
    > > My feeling is that these checks are still needed in the fcoe transport,
    > > but not in the LLDs. If someone can suggest a better way to protect
    > > against writable sysfs files when the module hasn't finished
    > > initializing, we should do that instead of the ifdefs.
    > >
    > > Hope this helps,
    > >
    > > //Rob
    > >
    > > FYI: mnc asked about this code and the trylock code in fcoe and libfcoe.
    > > I have patches in our internal validation to remove the trylock usage,
    > > but I don't have patches to fix the module state checking.
    > >
    > Yeah, this logic was from original fixing race condition in fcoe.ko, note
    > that we do need check the MODULE_STATE_LIVE, try_module_get() is not what
    > we wanted, plus module_is_live () checks if it is !GOING. Anyway, I don't
    > think this is needed any more for individual fcoe transport driver, e.g.,
    > fcoe.ko or bnx2fc, as the race is now for sysfs of libfcoe.
    >
    > I will send out a patch to clean up the fcoe.c for this.

    I agree. I'll follow it up with bnx2fc patch.

    Thanks,
    Bhanu
    >
    > Thanks,
    > yi
    >
    >
    >
    >
    >
    >
    > > --
    > > To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
    > > the body of a message to majordomo@vger.kernel.org
    > > More majordomo info at http://vger.kernel.org/majordomo-info.html



    --
    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/

    \
     
     \ /
      Last update: 2011-03-08 21:31    [W:0.029 / U:59.068 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site