lkml.org 
[lkml]   [2007]   [Oct]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 3/8] scsi: megaraid_sas - add module param max_sectors, cmd_per_lun
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:brking@linux.vnet.ibm.com]
> *Sent:* Fri 9/28/2007 3:30 PM
> *To:* Yang, Bo
> *Cc:* linux-scsi@vger.kernel.org; James.Bottomley@SteelEye.com;
> akpm@osdl.org; linux-kernel@vger.kernel.org; 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 majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

\
 
 \ /
  Last update: 2007-10-01 19:53    [W:0.444 / U:0.076 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site