Re: [PATCH 1/1] be2iscsi: Enabling MSIX and mcc_rings V2

[Mike Christie <michaelc@cs.wisc.edu>]

Adding Qlogic guys on this one, because I think they want something similar.

Jayamohan Kallickal wrote:
>  4) Adds sysfs for FW commands
> 

Jay, what operations are you wanting to do with the binary sysfs file? 
Are there any operations that should be common with Qlogic?

Also if you guys both need binary operations for card specific 
operations then binary sysfs is fine. However, James S had suggested 
using bsg for this too. Since that is upstream now, and you could just 
copy the FC implementation that might be a easier route (I think qlogic 
might have mentioned sysfs was a pain sometimes, and it is something you 
both could work on and help each other out on).



Comment on the implementation.

>  MODULE_DEVICE_TABLE(pci, beiscsi_pci_id_table);
>  
> +static ssize_t
> +beiscsi_sysfs_fw_wr(struct kobject *kobj, struct bin_attribute *bin_attr,
> +		    char *buf, loff_t off, size_t count) {
> +
> +	unsigned int ret_len = 0;
> +	struct Scsi_Host *shost = class_to_shost(container_of(kobj,
> +						 struct device, kobj));
> +	struct beiscsi_hba *phba = iscsi_host_priv(shost);
> +
> +	ret_len = mgmt_fw_cmd(&phba->ctrl, phba, buf, count);


For normal sysfs files I think we normally want it to do only one thing. 
This one seems to be a cmd to exec and the data buffer. I am not sure if 
that is right. Maybe you just want one file per operation. Or maybe you 
want another file for the length of the opertaion so you do not have to 
waste mem on that phba buffer.

Or JamesS's comment offlist about using the bsg pass through stuff is 
also ok. Adding the infrastucture is a little more work, but it is 
probably easier in the long run for the reasons he explained.



> +	if (!ret_len) {
> +		memcpy(buf, phba->attr_mem, count);
> +		return count;
> +	} else
> +		return ret_len;
> +}
> +
> +static ssize_t
> +beiscsi_sysfs_fw_rd(struct kobject *kobj, struct bin_attribute *bin_attr,
> +		    char *buf, loff_t off, size_t count) {
> +
> +	struct Scsi_Host *shost = class_to_shost(container_of(kobj,
> +						 struct device, kobj));
> +	struct beiscsi_hba *phba = iscsi_host_priv(shost);
> +
> +	memcpy(buf, phba->attr_mem, count);
> +	return count;
> +}
> +
> +static struct bin_attribute sysfs_drvr_fw_prgrm_attr = {
> +	.attr = {
> +		.name = "be2iscsi_fw_prgrm",
> +		.mode = S_IRUSR | S_IWUSR,
> +		.owner = THIS_MODULE,
> +	},
> +	.size = 0,
> +	.read = beiscsi_sysfs_fw_rd,
> +	.write = beiscsi_sysfs_fw_wr,
> +};
> +