On 03/12/2012 10:00 PM, Greg KH wrote:
> On Mon, Mar 12, 2012 at 04:09:31PM -0700, Robert Love wrote:
>> +static void fcoe_ctlr_attrs_release(struct device *dev)
>> +{
>> + struct fcoe_ctlr_attrs *ctlr = dev_to_ctlr(dev);
>> +
>> + put_device(ctlr->dev.parent);
>> + ctlr->dev.parent = NULL;
> You should never have to put a reference count on your parent, nor worry
> about setting this value to NULL. Just assign the parent when you
> register the device, no need to increment it.
Cool, I'll make these changes.
>> +#define fcoe_ctlr_id(x) \
>> + ((x)->id)
>> +#define fcoe_ctlr_work_q_name(x) \
>> + ((x)->work_q_name)
> <snip>
>
> Ick, what are all of these for, please don't do that.
>
These are only interesting when you look at the macros used to create
the show/store handlers for the attributes.
#define fcoe_ctlr_show_function(field, format_string, sz, cast) \
static ssize_t show_fcoe_ctlr_attrs_##field(struct device *dev, \
struct device_attribute
*attr, \
char *buf) \
{ \
struct fcoe_ctlr_attrs *ctlr = dev_to_ctlr(dev); \
if (ctlr->f->get_fcoe_ctlr_##field) \
ctlr->f->get_fcoe_ctlr_##field(ctlr); \
return snprintf(buf, sz, format_string, \
cast fcoe_ctlr_##field(ctlr)); \
}
It's the last functional line that matters. I might be able to change
that to something like:
cast ctlr->field
I'm pretty sure that won't compile, but if it did, it would only work
for the simple members of fcoe_ctlr_attrs. It gets a bit more
complicated because I have an embedded structure in fcoe_ctlr_attrs:
struct fcoe_ctlr_attrs {
u32 id;
...
struct fcoe_fc_els_lesb lesb;
};
So, when I want to use the fcoe_ctlr_show_function to build the 'show
handler' for this sysfs attribute I rely on the following:
#define fcoe_ctlr_link_fail(x) \
((x)->lesb.lesb_link_fail)
This allows my fcoe_ctlr_show_function macro to be generic and the fact
that the lesb members are within a sub-structure is contained within the
attribute's accessor macro.
I'm sure I could come up with a workaround, it would likely require me
to have a separate fcoe_ctlr_show_function for the lesb attributes. I
like that my fcoe_ctlr_show_function works for the simple
fcoe_ctlr_attrs members as well as for the embedded lesb members.
My feeling is that when you looked at the code you just saw unnecessary
accessors routines as their usage is not so obvious. I do not intend to
be using these accessors anywhere else other than the withing the
show/store building routines.
Given my explanation, do you still dislike these?
I could move them to the fcoe_sysfs.c so they're not in a header and
therefore would look less like accessors that developers should use...
Thanks, //Rob
PS: I'll also address your documentation concern before I repost.--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
