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
