Yang, Bo wrote:
> Brian,
>
> Thanks for your review. What is your objection to the current
> implementation by using bin_attribute?
Binary attributes are generally used for data which cannot
be parsed by the user. Since this attribute is simply a
single decimal value, it makes most sense to just use a regular
class_device_attribute. Additionally, if you use the
existing shost_attrs infrastructure in the scsi_host_template,
you can simplify your code, since you don't need to manually
create and destroy the sysfs files as scsi core will do this for
you.
-Brian
>
> Bo Yang
>
> ------------------------------------------------------------------------
> *From:* Brian King [mailto:[email protected]]
> *Sent:* Fri 9/28/2007 3:30 PM
> *To:* Yang, Bo
> *Cc:* [email protected]; [email protected];
> [email protected]; [email protected]; Patro, Sumant
> *Subject:* Re: [PATCH 3/8] scsi: megaraid_sas - add module param
> max_sectors, cmd_per_lun
>
> bo yang wrote:
>
>> +static ssize_t
>> +sysfs_max_sectors_read(struct kobject *kobj,
>> + struct bin_attribute *bin_attr,
>> + char *buf, loff_t off, size_t count)
>> +{
>> + struct Scsi_Host *host = class_to_shost(container_of(kobj,
>> + struct class_device, kobj));
>> + struct megasas_instance *instance =
>> + (struct megasas_instance *)host->hostdata;
>> +
>> + count = sprintf(buf, "%u\n", instance->max_sectors_per_req);
>> +
>> + return count+1;
>> +}
>> +
>> +static struct bin_attribute sysfs_max_sectors_attr = {
>> + .attr = {
>> + .name = "max_sectors",
>> + .mode = S_IRUSR|S_IRGRP|S_IROTH,
>> + .owner = THIS_MODULE,
>> + },
>> + .size = 7,
>> + .read = sysfs_max_sectors_read,
>> +};
>
> Why is this implemented as a binary sysfs attribute? Also, can you use
> the existing shost_attrs infrastructure that's in the scsi_host_template
> like megaraid_mbox uses?
>
>
>> +
>> + /*
>> + * Check if the module parameter value for max_sectors can be used
>> + */
>> + if (max_sectors && max_sectors <= instance->max_sectors_per_req)
>> + instance->max_sectors_per_req = max_sectors;
>> + else {
>> + if (max_sectors)
>> + printk(KERN_INFO
>> + "megasas: max_sectors should be > 0 and"
>> + "<= %d\n",
>> + instance->max_sectors_per_req);
>> + }
>
> Could be simplified to an else if, which would remove one indent level...
>
> -Brian
>
> --
> Brian King
> Linux on Power Virtualization
> IBM Linux Technology Center
>
--
Brian King
Linux on Power Virtualization
IBM Linux Technology Center
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Stuff]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
[Linux Resources]
