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
