linux-kernel - Re: [PATCH 3/8] scsi: megaraid_sas - add module param max_sectors, cmd_per

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [day] [month] [year] [list]

Message-ID: <4701335E.6080906@linux.vnet.ibm.com>
Date:	Mon, 01 Oct 2007 12:50:22 -0500
From:	Brian King <brking@...ux.vnet.ibm.com>
To:	"Yang, Bo" <Bo.Yang@....com>
CC:	linux-scsi@...r.kernel.org, James.Bottomley@...elEye.com,
	akpm@...l.org, linux-kernel@...r.kernel.org,
	"Patro, Sumant" <Sumant.Patro@....com>
Subject: Re: [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@...ux.vnet.ibm.com]
> *Sent:* Fri 9/28/2007 3:30 PM
> *To:* Yang, Bo
> *Cc:* linux-scsi@...r.kernel.org; James.Bottomley@...elEye.com;
> akpm@...l.org; linux-kernel@...r.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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/