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@xxxxxxxxxxxxxxxxxx] > *Sent:* Fri 9/28/2007 3:30 PM > *To:* Yang, Bo > *Cc:* linux-scsi@xxxxxxxxxxxxxxx; James.Bottomley@xxxxxxxxxxxx; > akpm@xxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; 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-scsi" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
