Re: [PATCH v4 4/9] snic:Add snic target discovery

[Hannes Reinecke <hare@suse.de>]

On 04/13/2015 07:25 AM, Narsimhulu Musini (nmusini) wrote:
> Hi Hannes,
> 
>   Thank you for reviewing patches. Please find responses inline.
> 
> 
> 
> On 09/04/15 6:29 pm, "Hannes Reinecke" <hare@suse.de> wrote:
> 
>>
>> On 04/09/2015 01:49 PM, Narsimhulu Musini wrote:
[ .. ]
>>> +/* snic_tgt_create: checks for existence of snic_tgt, if it doesn't
>>> + * it creates one.
>>> + */
>>> +static struct snic_tgt *
>>> +snic_tgt_create(struct snic *snic, struct snic_tgt_id *tgtid)
>>> +{
>>> +	struct snic_tgt *tgt = NULL;
>>> +	unsigned long flags;
>>> +	int ret;
>>> +
>>> +	tgt = snic_tgt_lookup(snic, tgtid);
>>> +	if (tgt) {
>>> +		/* update the information if required */
>>> +		return tgt;
>>> +	}
>>> +
>>> +	tgt = kzalloc(sizeof(*tgt), GFP_KERNEL);
>>> +	if (!tgt) {
>>> +		SNIC_HOST_ERR(snic->shost, "Failure to allocate snic_tgt.\n");
>>> +		ret = -ENOMEM;
>>> +
>>> +		return tgt;
>>> +	}
>>> +
>>> +	INIT_LIST_HEAD(&tgt->list);
>>> +	tgt->id = tgtid->tgt_id;
>>> +	tgt->channel = 0;
>>> +
>>> +	SNIC_BUG_ON(tgtid->tgt_type > SNIC_TGT_SAN);
>>> +	tgt->tdata.typ = tgtid->tgt_type;
>>> +
>>> +	/*
>>> +	 * Plugging into SML Device Tree
>>> +	 */
>>> +	tgt->tdata.disc_id = 0;
>>> +	tgt->state = SNIC_TGT_STAT_INIT;
>>> +	device_initialize(&tgt->dev);
>>> +	tgt->dev.parent = get_device(&snic->shost->shost_gendev);
>>> +	tgt->dev.release = snic_tgt_dev_release;
>> Why do you use your own scsi target instantiation here?
>> If it's equivalent to the scsi target than you should rather use the
>> 'scsi_target' structure here and attach driver-specific information
>> to either hostdata or starget_data.
> I got your idea, we followed an approach similar to fc (fc_rport_create()
> in scsi_transport_fc.c).
> Both are valid approaches. We opted for current approach, and gone through
> good amount of testing.
> I would like to keep the way it is. Please share your thoughts.

scsi_transport_fc() creates its own sysfs hierarchy for the remote
port, which is inserted between scsi_host and scsi_target:

/sys/devices/pci0000:00/0000:00:03.0/0000:07:00.3/host2/rport-2:0-13/target2:0:11/2:0:11:7

here, the fc_rport structure corresponds to the FC sysfs hierarchy,
containing all FC-related sysfs attributes. And as such makes sense
have a distinct object.

In your case no such hiearchy exists, and there are no additional
sysfs attributes which would warrant the creation of a distinct object.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		               zSeries & Storage
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html