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

[Aaron Lu <aaron.lu@intel.com>]

Hi Tejun,

On 01/09/2013 01:52 AM, Tejun Heo wrote:
> Hello, Aaron.
> 
> On Tue, Jan 08, 2013 at 05:07:46PM +0800, Aaron Lu wrote:
>>>> +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
>>
>> checkscript.sh doesn't seem like this if I put a tab around the ':'
>>
>> ERROR: spaces prohibited around that ':' (ctx:VxW)
>> #222: FILE: include/uapi/linux/cdrom.h:915:
>> +	__u8 reserved1:		2;
>>  	              ^
>>
>> Which style should I follow?
> 
> struct zpodd {
> 	bool			slot:1;
> 	bool			drawer:1;
> 
> 	struct ata_device	*dev;
> };

Oh, got it :-)

> 
>>> explaining synchronization.  Bitfields w/ their implicit RMW ops tend
>>> to make people wonder about what the access rules are.
>>
>> The slot and drawer bit field is assigned only once during ata probe
>> time in EH context, and accessed later in PM's callback context.
>> Not sure what access rule should I describe...
> 
> /* init during probe, RO afterwards */ should do but I'd prefer if you
> stay away from bitfields altogether.  There are cases where bitfields
> are okay but when you're working across multiple locking domains, it
> usually is a bad idea because the code which accesses those fields
> look completely independent while still being able to interact with
> each other.  They look properly synchronized until you realized they
> live on the same machine word.

Then I will not use bitfields, I was just thinking to save some space
and didn't realize it would bring such nasty problems...

> 
>>>> +/*
>>>> + * 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?
>>
>> There are other types of ODD other than slot and drawer, and both slot
>> and drawer type ODDs can be supported for ZPODD. So a bool can't convey
>> such information :-)
> 
> Then please make it a proper ATA enum and use it in struct zpodd too.

Sure, and thanks for all the suggestions!

-Aaron