Re: [PATCH v11 3/9] libata: identify and init ZPODD devices

[Tejun Heo <tj@kernel.org>]

On Sun, Jan 06, 2013 at 10:48:23AM +0800, Aaron Lu wrote:
> diff --git a/drivers/ata/libata-acpi.c b/drivers/ata/libata-acpi.c
> index ef01ac0..5aa7322 100644
> --- a/drivers/ata/libata-acpi.c
> +++ b/drivers/ata/libata-acpi.c
> @@ -1063,6 +1063,8 @@ void ata_acpi_bind(struct ata_device *dev)
>  
>  void ata_acpi_unbind(struct ata_device *dev)
>  {
> +	if (zpodd_dev_enabled(dev))
> +		zpodd_exit(dev);
>  	ata_acpi_remove_pm_notifier(dev);
>  	ata_acpi_unregister_power_resource(dev);
>  }

Wouldn't it make more sense to invoke zpodd_exit() from
ata_scsi_remove_dev() which is approximate counterpart of
dev_configure?

> +struct zpodd {
> +	bool slot:1;
> +	bool drawer:1;
> +
> +	struct ata_device *dev;
> +};

Field names are usually indented.  It would be nice to have a comment
explaining synchronization.  Bitfields w/ their implicit RMW ops tend
to make people wonder about what the access rules are.

> +static int run_atapi_cmd(struct ata_device *dev, const char *cdb,
> +		unsigned short cdb_len, char *buf, unsigned int buf_len)
> +{
> +	struct ata_taskfile tf = {0};

No need for 0.  { } is enough and more generic.

> +
> +	tf.flags |= ATA_TFLAG_ISADDR | ATA_TFLAG_DEVICE;
> +	tf.command = ATA_CMD_PACKET;
> +
> +	if (buf) {
> +		tf.protocol = ATAPI_PROT_PIO;
> +		tf.lbam = buf_len;
> +	} else {
> +		tf.protocol = ATAPI_PROT_NODATA;
> +	}
> +
> +	return ata_exec_internal(dev, &tf, cdb,
> +			buf ? DMA_FROM_DEVICE : DMA_NONE, buf, buf_len, 0);
> +}

So, the function name is a bit of misnomer given that ATAPI commands
are not limited to PIO or DMA_FROM_DEVICE.  Also, this function ends
up being used twice - once w/ read buffer and once w/o.  Do we really
want this function?  It's not like exec_internal is difficult to use.

> +/*
> + * Per the spec, only slot type and drawer type ODD can be supported
> + *
> + * Return 0 for slot type, 1 for drawer, -ENODEV for other types or on error.
> + */

Maybe bool odd_has_drawer() is better?

> +static int check_loading_mechanism(struct ata_device *dev)
> +{
> +	char buf[16];
> +	unsigned int ret;
> +	struct rm_feature_desc *desc = (void *)(buf + 8);
> +
> +	char cdb[] = {  GPCMD_GET_CONFIGURATION,
> +			2,      /* only 1 feature descriptor requested */
> +			0, 3,   /* 3, removable medium feature */
> +			0, 0, 0,/* reserved */
> +			0, sizeof(buf),
> +			0, 0, 0,
> +	};
> +
> +	ret = run_atapi_cmd(dev, cdb, sizeof(cdb), buf, sizeof(buf));
> +	if (ret)
> +		return -ENODEV;
> +
> +	if (be16_to_cpu(desc->feature_code) != 3)
> +		return -ENODEV;
> +
> +	if (desc->mech_type == 0 && desc->load == 0 && desc->eject == 1)
> +		return 0; /* slot */
> +	else if (desc->mech_type == 1 && desc->load == 0 && desc->eject == 1)
> +		return 1; /* drawer */
> +	else
> +		return -ENODEV;
> +}
> +
> +static bool odd_can_poweroff(struct ata_device *ata_dev)
> +{
> +	acpi_handle handle;
> +	acpi_status status;
> +	struct acpi_device *acpi_dev;
> +
> +	handle = ata_dev_acpi_handle(ata_dev);
> +	if (!handle)
> +		return false;
> +
> +	status = acpi_bus_get_device(handle, &acpi_dev);
> +	if (ACPI_FAILURE(status))
> +		return false;
> +
> +	return acpi_device_can_poweroff(acpi_dev);
> +}
> +
> +void zpodd_init(struct ata_device *dev)
> +{
> +	int ret;
> +	struct zpodd *zpodd;
> +
> +	if (dev->zpodd)
> +		return;
> +
> +	if (!odd_can_poweroff(dev))
> +		return;
> +
> +	if ((ret = check_loading_mechanism(dev)) == -ENODEV)
> +		return;
> +
> +	zpodd = kzalloc(sizeof(struct zpodd), GFP_KERNEL);
> +	if (!zpodd)
> +		return;
> +
> +	if (ret)
> +		zpodd->drawer = true;
> +	else
> +		zpodd->slot = true;
> +
> +	zpodd->dev = dev;
> +	dev->zpodd = zpodd;
> +}
> +
> +void zpodd_exit(struct ata_device *dev)
> +{
> +	kfree(dev->zpodd);
> +	dev->zpodd = NULL;
> +}
> diff --git a/drivers/ata/libata.h b/drivers/ata/libata.h
> index 7148a58..8cb4372 100644
> --- a/drivers/ata/libata.h
> +++ b/drivers/ata/libata.h
> @@ -230,4 +230,18 @@ static inline void ata_sff_exit(void)
>  { }
>  #endif /* CONFIG_ATA_SFF */
>  
> +/* libata-zpodd.c */
> +#ifdef CONFIG_SATA_ZPODD
> +void zpodd_init(struct ata_device *dev);
> +void zpodd_exit(struct ata_device *dev);
> +static inline bool zpodd_dev_enabled(struct ata_device *dev)
> +{
> +	return dev->zpodd ? true : false;

	return dev->zpodd or return dev->zpodd != NULL?

Other than the above nits, looks okay to me.

Thanks.

-- 
tejun