Re: [PATCH 11/17] target: add session sysfs class support

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

On Mon, Jun 08, 2020 at 02:02:16PM -0500, Mike Christie wrote:
> 
> 
> On 6/8/20 11:36 AM, Greg Kroah-Hartman wrote:
> > On Mon, Jun 08, 2020 at 10:35:56AM -0500, Mike Christie wrote:
> > > 
> > > 
> > > On 6/8/20 12:32 AM, Greg Kroah-Hartman wrote:
> > > > On Sun, Jun 07, 2020 at 03:35:58PM -0500, Mike Christie wrote:
> > > > > +int target_sysfs_add_session(struct se_portal_group *se_tpg,
> > > > > +			     struct se_session *se_sess)
> > > > > +{
> > > > > +	struct t10_transport_id *tpt_id = se_sess->tpt_id;
> > > > > +	int ret;
> > > > > +
> > > > > +	if (!try_module_get(se_sess->tfo->module))
> > > > > +		return -EINVAL;
> > > > > +
> > > > > +	ret = target_cp_endpoint_strs(se_tpg, se_sess);
> > > > > +	if (ret)
> > > > > +		goto put_mod;
> > > > > +
> > > > > +	se_sess->dev.groups = se_sess->tfo->session_attr_groups;
> > > > > +	ret = device_add(&se_sess->dev);
> > > > > +	if (ret) {
> > > > > +		pr_err("Could not add session%d to sysfs. Error %d.\n",
> > > > > +		       se_sess->sid, ret);
> > > > > +		goto free_ep_strs;
> > > > > +	}
> > > > > +
> > > > > +	pr_info("TCM added session-%d from [fabric: %s, target: %s, tpg %s, acl: %s] to [initiator port: %s%s%s]",
> > > > > +		se_sess->sid, se_sess->fabric_name, se_sess->target_name,
> > > > > +		se_sess->tpg_name, se_sess->acl_name ? se_sess->acl_name : "dynamic",
> > > > > +		tpt_id->name, tpt_id->session_id ? "," : "",
> > > > > +		tpt_id->session_id ? tpt_id->session_id : "");
> > > > 
> > > > You have a 'struct device', so please use it, no need for pr_info(),
> > > > always use the dev_*() calls instead.
> > > > 
> > > > but, when drivers and kernel code is all working properly, no need to be
> > > > noisy at all, this should just be a dev_dbg() call, right?
> > > 
> > > I liked the info one, because the the code can work correctly, but the
> > > remote devices we are connecting to are normally going to hit issues.
> > > 
> > > For example every time the storage network goes down temporarily the driver
> > > code will call remove function, then call the add again when it comes back
> > > up. Having it always logged helps us figure out the root problem later when
> > > the customer only has logs available.
> > 
> > Then make this a tracepoint or something, again, do not be noisy for
> > normal system operations.  Do you want this to be the case for all of
> 
> What's a special case vs normal?
> 
> I would agree having a SCSI HBA in your system and having it setup during
> system boot up is normal.
> 
> I would agree hardware failing is special.
> 
> Having a remote client connect to us, lose its connected then recover seems
> special, because it's a failure case.
> 
> Even the initial connection seems like a special event, because the user has
> done some extra steps to have the client connect to us. It's like adding a
> new disk to the system which we log today or like plugging in or removing a
> USB device which we also log.
> 
> 
> > your hardware devices all the time?
> > 
> 
> I do, but I'm a kernel developer :)
> 
> For the sys admin and distro debugging type of case, yes, they want this
> type of thing logged, because they have done some extra setup to have a
> remote client connect to us. When the connection is lost and then re-added
> they want all that info logged with the lower level info we are already
> logging, so they can follow the time of events.

Ok, then, if they really need this, use the standard format for logging
and use the dev_info() function instead.  That allows them to properly
determine what device/driver sent that message at that point in time.

thanks,

greg k-h