* [PATCH] [SCSI] bnx2fc: fix build error when !CONFIG_MODULES [not found] <20110302182740.e4dfd79f.sfr@canb.auug.org.au> @ 2011-03-02 22:10 ` Mariusz Kozlowski 2011-03-07 20:16 ` Mariusz Kozlowski 0 siblings, 1 reply; 9+ messages in thread From: Mariusz Kozlowski @ 2011-03-02 22:10 UTC (permalink / raw) To: Stephen Rothwell Cc: James E.J. Bottomley, Bhanu Prakash Gollapudi, linux-scsi, linux-kernel, linux-next, Mariusz Kozlowski 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) Signed-off-by: Mariusz Kozlowski <mk@lab.zgora.pl> --- drivers/scsi/bnx2fc/bnx2fc_fcoe.c | 4 ++++ 1 files changed, 4 insertions(+), 0 deletions(-) diff --git a/drivers/scsi/bnx2fc/bnx2fc_fcoe.c b/drivers/scsi/bnx2fc/bnx2fc_fcoe.c index e476e87..689db39 100644 --- a/drivers/scsi/bnx2fc/bnx2fc_fcoe.c +++ b/drivers/scsi/bnx2fc/bnx2fc_fcoe.c @@ -1812,10 +1812,12 @@ static int bnx2fc_disable(struct net_device *netdev) mutex_lock(&bnx2fc_dev_lock); +#ifdef CONFIG_SCSI_BNX2X_FCOE_MODULE if (THIS_MODULE->state != MODULE_STATE_LIVE) { rc = -ENODEV; goto nodev; } +#endif /* obtain physical netdev */ if (netdev->priv_flags & IFF_802_1Q_VLAN) @@ -1875,10 +1877,12 @@ static int bnx2fc_enable(struct net_device *netdev) BNX2FC_MISC_DBG("Entered %s\n", __func__); mutex_lock(&bnx2fc_dev_lock); +#ifdef CONFIG_SCSI_BNX2X_FCOE_MODULE if (THIS_MODULE->state != MODULE_STATE_LIVE) { rc = -ENODEV; goto nodev; } +#endif /* obtain physical netdev */ if (netdev->priv_flags & IFF_802_1Q_VLAN) -- 1.7.0.4 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] [SCSI] bnx2fc: fix build error when !CONFIG_MODULES 2011-03-02 22:10 ` [PATCH] [SCSI] bnx2fc: fix build error when !CONFIG_MODULES Mariusz Kozlowski @ 2011-03-07 20:16 ` Mariusz Kozlowski 2011-03-07 23:54 ` Bhanu Gollapudi 0 siblings, 1 reply; 9+ messages in thread From: Mariusz Kozlowski @ 2011-03-07 20:16 UTC (permalink / raw) To: Stephen Rothwell Cc: James E.J. Bottomley, Bhanu Prakash Gollapudi, linux-scsi, linux-kernel, linux-next, Mariusz Kozlowski 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..? -- Mariusz Kozlowski ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] [SCSI] bnx2fc: fix build error when !CONFIG_MODULES 2011-03-07 20:16 ` Mariusz Kozlowski @ 2011-03-07 23:54 ` Bhanu Gollapudi 2011-03-08 0:18 ` James Bottomley 0 siblings, 1 reply; 9+ messages in thread From: Bhanu Gollapudi @ 2011-03-07 23:54 UTC (permalink / raw) To: Mariusz Kozlowski Cc: Stephen Rothwell, James E.J. Bottomley, linux-scsi@vger.kernel.org, linux-kernel@vger.kernel.org, linux-next@vger.kernel.org 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. Thanks, Bhanu ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] [SCSI] bnx2fc: fix build error when !CONFIG_MODULES 2011-03-07 23:54 ` Bhanu Gollapudi @ 2011-03-08 0:18 ` James Bottomley 2011-03-08 1:09 ` Robert Love 0 siblings, 1 reply; 9+ messages in thread From: James Bottomley @ 2011-03-08 0:18 UTC (permalink / raw) To: Bhanu Gollapudi Cc: Mariusz Kozlowski, Stephen Rothwell, linux-scsi@vger.kernel.org, linux-kernel@vger.kernel.org, linux-next@vger.kernel.org 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? James ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] [SCSI] bnx2fc: fix build error when !CONFIG_MODULES 2011-03-08 0:18 ` James Bottomley @ 2011-03-08 1:09 ` Robert Love 2011-03-08 19:36 ` Zou, Yi 2011-03-10 15:32 ` James Bottomley 0 siblings, 2 replies; 9+ messages in thread From: Robert Love @ 2011-03-08 1:09 UTC (permalink / raw) To: James Bottomley Cc: Bhanu Gollapudi, Mariusz Kozlowski, Stephen Rothwell, linux-scsi@vger.kernel.org, linux-kernel@vger.kernel.org, linux-next@vger.kernel.org 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. ^ permalink raw reply [flat|nested] 9+ messages in thread
* RE: [PATCH] [SCSI] bnx2fc: fix build error when !CONFIG_MODULES 2011-03-08 1:09 ` Robert Love @ 2011-03-08 19:36 ` Zou, Yi 2011-03-08 20:29 ` Bhanu Gollapudi 2011-03-10 15:32 ` James Bottomley 1 sibling, 1 reply; 9+ messages in thread From: Zou, Yi @ 2011-03-08 19:36 UTC (permalink / raw) To: Love, Robert W, James Bottomley Cc: Bhanu Gollapudi, Mariusz Kozlowski, Stephen Rothwell, linux-scsi@vger.kernel.org, linux-kernel@vger.kernel.org, linux-next@vger.kernel.org > 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. 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* RE: [PATCH] [SCSI] bnx2fc: fix build error when !CONFIG_MODULES 2011-03-08 19:36 ` Zou, Yi @ 2011-03-08 20:29 ` Bhanu Gollapudi 0 siblings, 0 replies; 9+ messages in thread From: Bhanu Gollapudi @ 2011-03-08 20:29 UTC (permalink / raw) To: Zou, Yi Cc: Love, Robert W, James Bottomley, Mariusz Kozlowski, Stephen Rothwell, linux-scsi@vger.kernel.org, linux-kernel@vger.kernel.org, linux-next@vger.kernel.org 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] [SCSI] bnx2fc: fix build error when !CONFIG_MODULES 2011-03-08 1:09 ` Robert Love 2011-03-08 19:36 ` Zou, Yi @ 2011-03-10 15:32 ` James Bottomley 2011-03-12 2:03 ` Robert Love 1 sibling, 1 reply; 9+ messages in thread From: James Bottomley @ 2011-03-10 15:32 UTC (permalink / raw) To: Robert Love Cc: Bhanu Gollapudi, Mariusz Kozlowski, Stephen Rothwell, linux-scsi@vger.kernel.org, linux-kernel@vger.kernel.org, linux-next@vger.kernel.org On Mon, 2011-03-07 at 17:09 -0800, Robert Love 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. Well, what you've done isn't even fixing the race ... it's just narrowing the window. count checks on refcounted objects are almost always wrong. To see this just think what happens if the module goes dead the instruction cycle after you do the check. I don't understand the problem with the comments above. The way we solve the sysfs race in most systems (including modules) is to make sure they're initialised with the module and torn down as part of its exit process ... modules.c follows this pattern. The parameters are supposed to be initialised before the module has any state (because the init code may rely on them). I think the problem is you have what are essentially functional sysfs files done as module parameters in fcoe_transport.c ... this looks to be wrong. What you should have is these added as group attributes once the module is capable of processing them; that way you control the lifetimes. James > 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. > > -- > 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] [SCSI] bnx2fc: fix build error when !CONFIG_MODULES 2011-03-10 15:32 ` James Bottomley @ 2011-03-12 2:03 ` Robert Love 0 siblings, 0 replies; 9+ messages in thread From: Robert Love @ 2011-03-12 2:03 UTC (permalink / raw) To: James Bottomley Cc: Bhanu Gollapudi, Mariusz Kozlowski, Stephen Rothwell, linux-scsi@vger.kernel.org, linux-kernel@vger.kernel.org, linux-next@vger.kernel.org On Thu, 2011-03-10 at 07:32 -0800, James Bottomley wrote: > On Mon, 2011-03-07 at 17:09 -0800, Robert Love 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. > > Well, what you've done isn't even fixing the race ... it's just > narrowing the window. count checks on refcounted objects are almost > always wrong. To see this just think what happens if the module goes > dead the instruction cycle after you do the check. > > I don't understand the problem with the comments above. The way we > solve the sysfs race in most systems (including modules) is to make sure > they're initialised with the module and torn down as part of its exit > process ... modules.c follows this pattern. The parameters are supposed > to be initialised before the module has any state (because the init code > may rely on them). > > I think the problem is you have what are essentially functional sysfs > files done as module parameters in fcoe_transport.c ... this looks to be > wrong. What you should have is these added as group attributes once the > module is capable of processing them; that way you control the > lifetimes. You're right, this is our problem. Unfortunately, we're in a bit of a "chicken and egg" situation. For SW FCoE, we don't know which netdevice the user wants to run FCoE traffic on so we don't create any devices or other sysfs entries until the user tells us to 'create'. The user passes the netdevice name to our sysfs file and then we create the scsi_host/fc_host. I am not sure what I would attach a group of attributes (create, destroy, etc...) to. If I had per-instance devices I could add attributes to them, but I need the 'create' interface before I can create any per-instance devices. One option would be to go back to what we used to do (pre-mainline acceptance) which is the following: static const struct kobj_attribute fcoe_destroyattr = \ __ATTR(destroy, S_IWUSR, NULL, fcoe_destroy); static const struct kobj_attribute fcoe_createattr = \ __ATTR(create, S_IWUSR, NULL, fcoe_create); static int __init fcoe_init(void) { ... rc = sysfs_create_file(&THIS_MODULE->mkobj.kobj, &fcoe_destroyattr.attr); if (!rc) rc = sysfs_create_file(&THIS_MODULE->mkobj.kobj, &fcoe_createattr.attr) ... } I think this is bad because we're manipulating kobjects too directly, right? A second option would be to keep everything the same but use an atomic bit to signify when the module has completed initialization. We'd still need a check in each of the sysfs entries store routines, but we wouldn't be checking against the module state. A third option is to do something like bonding does, where it creates a /sys/class/net/bonding_masters entry. My guess is that this would be controversial; adding fcoe interfaces class/net could be considered polluting the networking space with storage information. A fourth option would be to drop sysfs all together and use netlink, like iSCSI. Currently we don't need an extensive kernel/user interface, so I look at this as overkill. I suppose the second option would solve the current race. I realize it doesn't necessarily fix the fact that we're adding usable sysfs files prematurely, but I'm not sure where I'd attach the attributes if I were do do it at the end of libfcoe's module_init routine. I'll mail my patch for comments. Thanks, //Rob ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2011-03-12 2:03 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20110302182740.e4dfd79f.sfr@canb.auug.org.au>
2011-03-02 22:10 ` [PATCH] [SCSI] bnx2fc: fix build error when !CONFIG_MODULES Mariusz Kozlowski
2011-03-07 20:16 ` Mariusz Kozlowski
2011-03-07 23:54 ` Bhanu Gollapudi
2011-03-08 0:18 ` James Bottomley
2011-03-08 1:09 ` Robert Love
2011-03-08 19:36 ` Zou, Yi
2011-03-08 20:29 ` Bhanu Gollapudi
2011-03-10 15:32 ` James Bottomley
2011-03-12 2:03 ` Robert Love
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox