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
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
