Re: [RFC PATCH v2 3/3] ACPI / device_sysfs: Add eject_show and add a cancel option in eject_store

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Greg,

On Fri, Jan 03, 2020 at 09:37:28AM +0100, gregkh@xxxxxxxxxxxxxxxxxxx wrote:
> On Fri, Jan 03, 2020 at 04:40:17AM +0000, Chester Lin wrote:
> > Add an eject_show attribute for users to monitor current status because
> > sometimes it could take time to finish an ejection so we need to know
> > whether it is still in progress or not. For userspace who might need to
> > cancel an onging ejection, we also offer an option in eject_store.
> > 
> > Signed-off-by: Chester Lin <clin@xxxxxxxx>
> > ---
> >  Documentation/ABI/testing/sysfs-bus-acpi |  9 ++-
> >  drivers/acpi/device_sysfs.c              | 94 +++++++++++++++++++++---
> >  drivers/acpi/internal.h                  |  4 +-
> >  drivers/acpi/scan.c                      | 38 +++++++++-
> >  4 files changed, 129 insertions(+), 16 deletions(-)
> > 
> > diff --git a/Documentation/ABI/testing/sysfs-bus-acpi b/Documentation/ABI/testing/sysfs-bus-acpi
> > index e7898cfe5fb1..32fdf4af962e 100644
> > --- a/Documentation/ABI/testing/sysfs-bus-acpi
> > +++ b/Documentation/ABI/testing/sysfs-bus-acpi
> > @@ -53,9 +53,12 @@ What:		/sys/bus/acpi/devices/.../eject
> >  Date:		December 2006
> >  Contact:	Rafael J. Wysocki <rjw@xxxxxxxxxxxxx>
> >  Description:
> > -		Writing 1 to this attribute will trigger hot removal of
> > -		this device object.  This file exists for every device
> > -		object that has _EJ0 method.
> > +		(R) Allows users to read eject status of the device object.
> > +		(W) Writing 1 to this attribute will trigger hot removal of
> > +		this device object. Writing 2 to this attribute will cancel hot
> > +		removal work if it's still in offline process and the original
> > +		state of this device object will be recovered. This file exists
> > +		for every device object that has _EJ0 method.
> 
> Oh that's going to cause problems :)
> 
> Why not just have a new file, "cancel_eject" that you can write to, to
> cancel the eject happening?
> 
> That way you don't have an odd "state" that this file needs to keep
> track of.
> 

Thank you for the advice and I will add it.

> And what happens if you write a '2' here when eject is not happening?
> It didn't look like your code handled that state well.
>

When eject is not happening, the eject_stat is null so the procedure will go to
eject_end without changing anything.

> >  
> >  What:		/sys/bus/acpi/devices/.../status
> >  Date:		Jan, 2014
> > diff --git a/drivers/acpi/device_sysfs.c b/drivers/acpi/device_sysfs.c
> > index 96869f1538b9..6801b268fe9d 100644
> > --- a/drivers/acpi/device_sysfs.c
> > +++ b/drivers/acpi/device_sysfs.c
> > @@ -365,17 +365,13 @@ static ssize_t power_state_show(struct device *dev,
> >  
> >  static DEVICE_ATTR_RO(power_state);
> >  
> > -static ssize_t
> > -acpi_eject_store(struct device *d, struct device_attribute *attr,
> > -		const char *buf, size_t count)
> > +static ssize_t eject_show(struct device *d,
> > +				struct device_attribute *attr, char *buf)
> >  {
> >  	struct acpi_device *acpi_device = to_acpi_device(d);
> >  	acpi_object_type not_used;
> >  	acpi_status status;
> >  
> > -	if (!count || buf[0] != '1')
> > -		return -EINVAL;
> > -
> >  	if ((!acpi_device->handler || !acpi_device->handler->hotplug.enabled)
> >  	    && !acpi_device->driver)
> >  		return -ENODEV;
> > @@ -384,18 +380,96 @@ acpi_eject_store(struct device *d, struct device_attribute *attr,
> >  	if (ACPI_FAILURE(status) || !acpi_device->flags.ejectable)
> >  		return -ENODEV;
> >  
> > +	return sprintf(buf, "%s\n", acpi_eject_status_string(acpi_device));
> > +}
> > +
> > +static ssize_t
> > +eject_store(struct device *d, struct device_attribute *attr,
> > +		const char *buf, size_t count)
> > +{
> > +	struct acpi_device *acpi_device = to_acpi_device(d);
> > +	struct eject_data *eject_node = NULL;
> > +	acpi_object_type not_used;
> > +	acpi_status status;
> > +	bool cancel_eject = false;
> > +	ssize_t ret;
> > +
> > +	if (!count)
> > +		return -EINVAL;
> > +
> > +	switch (buf[0]) {
> > +	case '1':
> > +		break;
> > +	case '2':
> > +		acpi_scan_lock_acquire();
> > +		eject_node = (struct eject_data *)acpi_device->eject_stat;
> > +
> > +		if (!eject_node) {
> > +			acpi_scan_lock_release();
> > +			ret = -EINVAL;
> > +			goto eject_end;
> > +		}
> > +
> > +		/*
> > +		 * Find a root to start cancellation from the top
> > +		 */
> > +		if (eject_node->base.root_handle) {
> > +			acpi_device = acpi_bus_get_acpi_device(
> > +					eject_node->base.root_handle);
> > +
> > +			if (acpi_device)
> > +				eject_node =
> > +				   (struct eject_data *)acpi_device->eject_stat;
> > +			else
> > +				eject_node = NULL;
> > +
> > +		}
> > +
> > +		if (eject_node &&
> > +		   (eject_node->status == ACPI_EJECT_STATUS_GOING_OFFLINE ||
> > +		    eject_node->status == ACPI_EJECT_STATUS_READY_REMOVE)) {
> > +			eject_node->status = ACPI_EJECT_STATUS_CANCEL;
> > +			cancel_eject = true;
> > +		}
> > +
> > +		acpi_scan_lock_release();
> > +		if (cancel_eject)
> > +			break;
> > +	default:
> > +		ret = -EINVAL;
> > +		goto eject_end;
> > +	};
> > +
> > +	if ((!acpi_device->handler || !acpi_device->handler->hotplug.enabled)
> > +	    && !acpi_device->driver) {
> > +		ret = -ENODEV;
> > +		goto eject_end;
> > +	}
> > +
> > +	status = acpi_get_type(acpi_device->handle, &not_used);
> > +	if (ACPI_FAILURE(status) || !acpi_device->flags.ejectable) {
> > +		ret = -ENODEV;
> > +		goto eject_end;
> > +	}
> > +
> >  	get_device(&acpi_device->dev);
> >  	status = acpi_hotplug_schedule(acpi_device, ACPI_OST_EC_OSPM_EJECT);
> > -	if (ACPI_SUCCESS(status))
> > -		return count;
> > +	if (ACPI_SUCCESS(status)) {
> > +		ret = count;
> > +		goto eject_end;
> > +	}
> >  
> >  	put_device(&acpi_device->dev);
> >  	acpi_evaluate_ost(acpi_device->handle, ACPI_OST_EC_OSPM_EJECT,
> >  			  ACPI_OST_SC_NON_SPECIFIC_FAILURE, NULL);
> > -	return status == AE_NO_MEMORY ? -ENOMEM : -EAGAIN;
> > +	ret = (status == AE_NO_MEMORY) ? -ENOMEM : -EAGAIN;
> > +
> > +eject_end:
> > +	return ret;
> > +
> >  }
> >  
> > -static DEVICE_ATTR(eject, 0200, NULL, acpi_eject_store);
> > +static DEVICE_ATTR_RW(eject);
> >  
> >  static ssize_t
> >  acpi_device_hid_show(struct device *dev, struct device_attribute *attr, char *buf)
> > diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h
> > index 8154690b872b..e5d526402188 100644
> > --- a/drivers/acpi/internal.h
> > +++ b/drivers/acpi/internal.h
> > @@ -265,7 +265,8 @@ enum acpi_eject_status {
> >  	ACPI_EJECT_STATUS_NONE = 0,
> >  	ACPI_EJECT_STATUS_GOING_OFFLINE,
> >  	ACPI_EJECT_STATUS_READY_REMOVE,
> > -	ACPI_EJECT_STATUS_FAIL
> > +	ACPI_EJECT_STATUS_FAIL,
> > +	ACPI_EJECT_STATUS_CANCEL
> >  };
> >  
> >  enum acpi_eject_node_type {
> > @@ -286,5 +287,6 @@ struct eject_data {
> >  };
> >  
> >  void acpi_eject_retry(struct acpi_device *adev);
> > +char *acpi_eject_status_string(struct acpi_device *adev);
> >  
> >  #endif /* _ACPI_INTERNAL_H_ */
> > diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> > index 13f16b6ad7a2..90983c067410 100644
> > --- a/drivers/acpi/scan.c
> > +++ b/drivers/acpi/scan.c
> > @@ -366,8 +366,9 @@ static int acpi_scan_offline_check(struct acpi_device *device)
> >  			return -EBUSY;
> >  		}
> >  
> > -		if (eject_obj->status == ACPI_EJECT_STATUS_FAIL) {
> > -			dev_warn(&device->dev, "Eject failed. Recover all.\n");
> > +		if (eject_obj->status == ACPI_EJECT_STATUS_FAIL ||
> > +		    eject_obj->status == ACPI_EJECT_STATUS_CANCEL) {
> > +			dev_warn(&device->dev, "Eject stopped. Recover all.\n");
> >  			acpi_scan_notify_online(device);
> >  			return -EAGAIN;
> >  		}
> > @@ -383,6 +384,39 @@ static int acpi_scan_offline_check(struct acpi_device *device)
> >  	return ret;
> >  }
> >  
> > +char *acpi_eject_status_string(struct acpi_device *adev)
> > +{
> > +	struct eject_data *eject_obj;
> > +	char *status_string = "none";
> > +
> > +	mutex_lock(&acpi_scan_lock);
> > +	eject_obj = (struct eject_data *) adev->eject_stat;
> 
> Always use checkpatch.pl so maintainers don't have to ask you to use
> checkpatch.pl :)
> 

Acutally I have used checkpatch.pl but haven't found a warning on this
line:
  total: 0 errors, 0 warnings, 199 lines checked

The last commit of checkpatch.pl in my code base is 184b8f7f91ca. Anyway,
thank you for the reminder I will remove the cast from next version :)

> > +
> > +	if (eject_obj) {
> > +		switch (eject_obj->status) {
> > +		case ACPI_EJECT_STATUS_NONE:
> > +			break;
> > +		case ACPI_EJECT_STATUS_GOING_OFFLINE:
> > +			status_string = "going offline";
> > +			break;
> > +		case ACPI_EJECT_STATUS_READY_REMOVE:
> > +			status_string = "ready to remove";
> > +			break;
> > +		case ACPI_EJECT_STATUS_FAIL:
> > +			status_string = "failure";
> > +			break;
> > +		case ACPI_EJECT_STATUS_CANCEL:
> > +			status_string = "cancel";
> > +			break;
> > +		default:
> > +			status_string = "(unknown)";
> > +		}
> > +	}
> > +
> > +	mutex_unlock(&acpi_scan_lock);
> > +	return status_string;
> 
> So the state can change right after checking it and reporting it to
> userspace?
> 
> If so, what good is the lock here at all?
> 

I use this lock to prevent the eject_state from being freed during a query
since any status change might still happen. For example, the userland process
could be preempted before accessing eject_state and then the eject_state could
be quickly freed due to eject failure or eject completion. [The free opertion
is implemented in acpi_free_eject_stat()]

> 
> thanks,
> 
> greg k-h
> 



[Index of Archives]     [Linux IBM ACPI]     [Linux Power Management]     [Linux Kernel]     [Linux Laptop]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Device Mapper]     [Linux Resources]

  Powered by Linux