RE: [PATCH 08/21] usb/gadget: f_mass_storage: split fsg_common initialization into a number of functions

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, 17 Jul 2013, Andrzej Pietrasiewicz wrote:

> Hello Alan,

Hello.

> > > @@ -179,7 +179,7 @@ EXPORT_SYMBOL(fsg_ss_function);  void
> > > fsg_lun_close(struct fsg_lun *curlun)  {
> > >  	if (curlun->filp) {
> > > -		LDBG(curlun, "close backing file\n");
> > > +		pr_debug("close backing file\n");
> > 
> > Here and in lots of other places, you have replaced a log message that
> > includes the LUN's name and driver with a message that includes
> > neither.  If somebody reads the system log after this change, how are
> > they going to know _which_ LUN had its backing file closed?  That
> > information should be included in the message.
> > 
> 
> I should have been more explicit in the commit log.
> My main concern with the LDBG and its friends is that they need
> to have a device to operate on.
> 
> So far this hasn't been an issue, because the luns have been
> represented in sysfs. By the way, using sysfs for this kind
> of things could be considered "abuse". In a configfs-based
> gadget the luns are represented in configfs instead,
> so there is no device to use for dev_dbg and similar;
> the devices are used in the legacy g_mass_storage.ko, though.
> 
> As far as I can tell the only reason each lun contains a struct device
> is for it to appear in sysfs, and the only reason for it to appear
> in sysfs is to have a file-like attribute for the user to read from/
> write to. Now that configfs is taking over this responsibility,
> there is no need for struct device.  This is what I think,
> I guess that Michal's explanation would be authoritative.
> 
> @Michal: Could you throw some light on the subject?

That's okay; I originally wrote the code that registers LUNs in sysfs.  
You're right; I did it so that there would be file-like attributes for
the user to access.  With configfs the device structures aren't needed.

> Meanwhile I kept the struct device in struct fsg_lun
> for the sake of Compatibility and it should be kept until
> the g_xxxx.ko modules are dropped altogether
> (which might be a long time).
> 
> > Why not just change the definitions of LDBG, LINFO, and so on instead
> > of changing all the places where they get called?
> > 
> 
> If lun's device is to be omitted, there is no point in passing luns
> to the said macros, which means changing each macro invocation anyway.
> However, if a lun were useful in the macro your suggestion is
> absolutely right.

I strongly feel that the macros should print out the LUN's name as 
a prefix to the rest of the message.  Indeed, if there are several 
instances of the mass-storage function running (perhaps in multiple 
interfaces or attached to multiple UDCs) then the prefix should also 
indicate the instance the LUN belongs to.

These names don't have to be device names.  But they should be 
sufficient to identify the particular LUN the message is about.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux