diff mbox

ses: do not add a device to an enclosure if enclosure_add_links() fails.

Message ID 1498557207-12364-1-git-send-email-mlombard@redhat.com (mailing list archive)
State Accepted, archived
Headers show

Commit Message

Maurizio Lombardi June 27, 2017, 9:53 a.m. UTC
The enclosure_add_device() function should fail if it can't
create the relevant sysfs links.

Signed-off-by: Maurizio Lombardi <mlombard@redhat.com>
---
 drivers/misc/enclosure.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

Comments

Douglas Miller June 27, 2017, 12:50 p.m. UTC | #1
On 06/27/2017 04:53 AM, Maurizio Lombardi wrote:
> The enclosure_add_device() function should fail if it can't
> create the relevant sysfs links.
>
> Signed-off-by: Maurizio Lombardi <mlombard@redhat.com>
> ---
>   drivers/misc/enclosure.c | 14 ++++++++++----
>   1 file changed, 10 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/misc/enclosure.c b/drivers/misc/enclosure.c
> index d3fe3ea..eb29113 100644
> --- a/drivers/misc/enclosure.c
> +++ b/drivers/misc/enclosure.c
> @@ -375,6 +375,7 @@ int enclosure_add_device(struct enclosure_device *edev, int component,
>   			 struct device *dev)
>   {
>   	struct enclosure_component *cdev;
> +	int err;
>
>   	if (!edev || component >= edev->components)
>   		return -EINVAL;
> @@ -384,12 +385,17 @@ int enclosure_add_device(struct enclosure_device *edev, int component,
>   	if (cdev->dev == dev)
>   		return -EEXIST;
>
> -	if (cdev->dev)
> +	if (cdev->dev) {
>   		enclosure_remove_links(cdev);
> -
> -	put_device(cdev->dev);
> +		put_device(cdev->dev);
> +	}
>   	cdev->dev = get_device(dev);
> -	return enclosure_add_links(cdev);
> +	err = enclosure_add_links(cdev);
> +	if (err) {
> +		put_device(cdev->dev);
> +		cdev->dev = NULL;
> +	}
> +	return err;
>   }
>   EXPORT_SYMBOL_GPL(enclosure_add_device);
>
Tested-by: Douglas Miller <dougmill@linux.vnet.ibm.com>

This fixes a problem where udevd (insmod ses) races with/overtakes 
do_scan_async(), which creates the directory target of the symlink, 
resulting in missing enclosure symlinks. This patch relaxes the symlink 
creation allowing for delayed addition to enclosure and creation of 
symlinks after do_scan_async() has created the target directory.
Martin K. Petersen June 28, 2017, 1:10 a.m. UTC | #2
Maurizio,

> The enclosure_add_device() function should fail if it can't
> create the relevant sysfs links.

James?
James Bottomley June 28, 2017, 1:58 a.m. UTC | #3
On Tue, 2017-06-27 at 21:10 -0400, Martin K. Petersen wrote:
> Maurizio,
> 
> > 
> > The enclosure_add_device() function should fail if it can't
> > create the relevant sysfs links.
> 
> James?

It's essentially the patch I proposed, so I'm fine with it.  Although I
would like more root cause analysis about why we have this problem.

James
Martin K. Petersen July 1, 2017, 8:53 p.m. UTC | #4
Maurizio,

> The enclosure_add_device() function should fail if it can't
> create the relevant sysfs links.

Applied to 4.13/scsi-queue, thanks!
Douglas Miller July 10, 2017, 1:27 p.m. UTC | #5
On 06/27/2017 07:50 AM, Douglas Miller wrote:
> On 06/27/2017 04:53 AM, Maurizio Lombardi wrote:
>> The enclosure_add_device() function should fail if it can't
>> create the relevant sysfs links.
>>
>> Signed-off-by: Maurizio Lombardi <mlombard@redhat.com>
>> ---
>>   drivers/misc/enclosure.c | 14 ++++++++++----
>>   1 file changed, 10 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/misc/enclosure.c b/drivers/misc/enclosure.c
>> index d3fe3ea..eb29113 100644
>> --- a/drivers/misc/enclosure.c
>> +++ b/drivers/misc/enclosure.c
>> @@ -375,6 +375,7 @@ int enclosure_add_device(struct enclosure_device 
>> *edev, int component,
>>                struct device *dev)
>>   {
>>       struct enclosure_component *cdev;
>> +    int err;
>>
>>       if (!edev || component >= edev->components)
>>           return -EINVAL;
>> @@ -384,12 +385,17 @@ int enclosure_add_device(struct 
>> enclosure_device *edev, int component,
>>       if (cdev->dev == dev)
>>           return -EEXIST;
>>
>> -    if (cdev->dev)
>> +    if (cdev->dev) {
>>           enclosure_remove_links(cdev);
>> -
>> -    put_device(cdev->dev);
>> +        put_device(cdev->dev);
>> +    }
>>       cdev->dev = get_device(dev);
>> -    return enclosure_add_links(cdev);
>> +    err = enclosure_add_links(cdev);
>> +    if (err) {
>> +        put_device(cdev->dev);
>> +        cdev->dev = NULL;
>> +    }
>> +    return err;
>>   }
>>   EXPORT_SYMBOL_GPL(enclosure_add_device);
>>
> Tested-by: Douglas Miller <dougmill@linux.vnet.ibm.com>
>
> This fixes a problem where udevd (insmod ses) races with/overtakes 
> do_scan_async(), which creates the directory target of the symlink, 
> resulting in missing enclosure symlinks. This patch relaxes the 
> symlink creation allowing for delayed addition to enclosure and 
> creation of symlinks after do_scan_async() has created the target 
> directory.
>
Has there been any progress with getting this patch accepted?
Maurizio Lombardi July 10, 2017, 1:46 p.m. UTC | #6
Douglas,

> Has there been any progress with getting this patch accepted?
> 

It has been merged already.
It's in linux-next, commit 62e62ffd95539b9220894a7900a619e0f3ef4756

https://kernel.googlesource.com/pub/scm/linux/kernel/git/next/linux-next/+/62e62ffd95539b9220894a7900a619e0f3ef4756

Maurizio.
diff mbox

Patch

diff --git a/drivers/misc/enclosure.c b/drivers/misc/enclosure.c
index d3fe3ea..eb29113 100644
--- a/drivers/misc/enclosure.c
+++ b/drivers/misc/enclosure.c
@@ -375,6 +375,7 @@  int enclosure_add_device(struct enclosure_device *edev, int component,
 			 struct device *dev)
 {
 	struct enclosure_component *cdev;
+	int err;
 
 	if (!edev || component >= edev->components)
 		return -EINVAL;
@@ -384,12 +385,17 @@  int enclosure_add_device(struct enclosure_device *edev, int component,
 	if (cdev->dev == dev)
 		return -EEXIST;
 
-	if (cdev->dev)
+	if (cdev->dev) {
 		enclosure_remove_links(cdev);
-
-	put_device(cdev->dev);
+		put_device(cdev->dev);
+	}
 	cdev->dev = get_device(dev);
-	return enclosure_add_links(cdev);
+	err = enclosure_add_links(cdev);
+	if (err) {
+		put_device(cdev->dev);
+		cdev->dev = NULL;
+	}
+	return err;
 }
 EXPORT_SYMBOL_GPL(enclosure_add_device);