[openib-general] [PATCH 06/12] SRP: Changing ibsrpdm

[Ishai Rabinovitz]

On Mon, May 01, 2006 at 04:50:32PM +0300, Muli Ben-Yehuda wrote:
> On Mon, May 01, 2006 at 02:28:48PM +0300, Ishai Rabinovitz wrote:
> > 
> > Support a display of list of target from user level.
> > 
> > Signed-off-by: Ishai Rabinovitz <ishai at mellanox.co.il>
> > Index: last_stable/drivers/infiniband/ulp/srp/ib_srp.c
> > ===================================================================
> > --- last_stable.orig/drivers/infiniband/ulp/srp/ib_srp.c	2006-04-21 01:13:04.000000000 +0300
> > +++ last_stable/drivers/infiniband/ulp/srp/ib_srp.c	2006-04-21 03:56:05.000000000 +0300
> > @@ -1730,6 +1730,63 @@ end:
> >  
> >  static CLASS_DEVICE_ATTR(remove_target, S_IWUSR, NULL, srp_remove_target);
> >  
> > +#define TARGET_INFO_BUF_SIZE 126
> > +
> > +static ssize_t list_targets(struct class_device *class_dev, char *buf)
> > +{
> > +	struct srp_host *host =
> > +		container_of(class_dev, struct srp_host, class_dev);
> > +	struct srp_target_port *target;
> > +	int printed=0, ret;
> > +
> > +	mutex_lock(&host->target_mutex);
> > +	list_for_each_entry(target, &host->target_list, list)
> 
> Can this race with list addition / removal? I saw that you removed the
> lock in an earlier patch?

No, In an erlier patch I did not removed the lock, I enlarged it scope to include 
the entire call to srp_find_target.

> 
> > +		if (target->state == SRP_TARGET_LIVE) {
> 
> You'd have an easier time with the indentation if you'd do 
> 
> if (target->state != SRP_TARGET_LIVE)
>         continue;
> 
> here
> 
> > +			ret = sprintf(buf+printed,
> > +				"id_ext=%016llx,ioc_guid=%016llx,"
> > +				"dgid=%04x%04x%04x%04x%04x%04x%04x%04x,"
> > +				"pkey=%04x,service_id=%016llx\n",
> > +				(unsigned long long)
> > +					be64_to_cpu(target->id_ext),
> > +				(unsigned long long)
> > +					be64_to_cpu(target->ioc_guid),
> > +				(int) be16_to_cpu(*(__be16 *)
> > +					&target->path.dgid.raw[0]),
> > +				(int) be16_to_cpu(*(__be16 *)
> > +					&target->path.dgid.raw[2]),
> > +				(int) be16_to_cpu(*(__be16 *)
> > +					&target->path.dgid.raw[4]),
> > +				(int) be16_to_cpu(*(__be16 *)
> > +					&target->path.dgid.raw[6]),
> > +				(int) be16_to_cpu(*(__be16 *)
> > +					&target->path.dgid.raw[8]),
> > +				(int) be16_to_cpu(*(__be16 *)
> > +					&target->path.dgid.raw[10]),
> > +				(int) be16_to_cpu(*(__be16 *)
> > +					&target->path.dgid.raw[12]),
> > +				(int) be16_to_cpu(*(__be16 *)
> > +					&target->path.dgid.raw[14]),
> 
> This is pretty horrible - could you use show_dgid() here?

Id will add a redundant copy of the buffer.
> 
> Cheers,
> Muli

-- 
Ishai Rabinovitz