On Sat, 26 Nov 2005, Pavel Machek wrote:
> Hi!
>
> > > This converts comments into C-style,
> >
> > You missed a lot of them.
>
> Yes, I know, at one point I realized you probably liked it that
> way. Shall I resent just the "remove the initializer" part?
Yes, okay. If nobody really minds the C++ comments, I'd prefer to leave
them in.
> > What's the reason for this prejudice against C++-style comments in the
> > kernel? I've never understood it... They seem so obviously useful,
> > saving 3 characters of typing at the end of each comment line.
>
> I've never seen a piece of non-crappy code using them... that's
> probably it. They tend to come from C++ and C++ code tends to be ****.
That's understandable. I feel the same way about
VeryLongAndHardToReadIdentifierNames.
> > > It uses 'volatile' quite heavily, and that scares me a lot.
> >
> > Alan Cox didn't like it either, but he wasn't able to give a clear
> > explanation why not. He just mumbled something about C compilers
> > generating bad code for volatiles...
>
> It probably does not what you want it to. For example:
>
> static int fsg_setup(struct usb_gadget *gadget,
> const struct usb_ctrlrequest *ctrl)
> {
> struct fsg_dev *fsg = get_gadget_data(gadget);
> int rc;
> int w_length = le16_to_cpu(ctrl->wLength);
>
> ++fsg->ep0_req_tag; // Record arrival of a new request
> fsg->ep0req->context = NULL;
> fsg->ep0req->length = 0;
> dump_msg(fsg, "ep0-setup", (u8 *) ctrl, sizeof(*ctrl));
>
> Do you realize that it is okay for compiler to move assignment into
> context before increase of req_tag? What you probably want is
>
> ++fsg->ep0_req_tag; // Record arrival of a new request
> mb();
> fsg->ep0req->context = NULL;
> fsg->ep0req->length = 0;
> dump_msg(fsg, "ep0-setup", (u8 *) ctrl, sizeof(*ctrl));
>
> ...or something like that. Or perhaps atomic_inc(), that should have
> some barriers built-in.
Yes, I did realize it. The reordering issues weren't important, as far as
I can recall. Maybe the only reason I used those volatiles was for the
implied compiler barriers; by now I've forgotten what the actual reasons
were.
It does deserve to be cleaned up. I'll go over the driver and get rid
of those volatiles, or replace them with explicit barriers where needed.
By the way, what brought on this sudden interest in g_file_storage?
On Sat, 26 Nov 2005, Pete Zaitcev wrote:
> Alan, the "volatile" keyword has no meaning in kernel and should never be
> used (most especially not in drivers). It is defined in context of
> single-threaded computation interrupted by signals, so it has no sense
> where SMP is involved.
I disagree. The original use was for contexts where the data values
were actually device registers, subject to unpredictable and asynchronous
changes by the hardware -- which is very similar to what one deals with in
an SMP environment, except that now the "device hardware" is actually
another CPU.
In general, volatile is intended for use in a context where multiple
entities (or a single multi-threaded processor) can operate simultaneously
on the same data. It does not address all the issues involved because it
ignores hardware-level CPU reordering effects. But it does act as a sort
of permanent compiler barrier surrounding all uses of a data value.
Those are the main reasons Alan Cox didn't like it: lack of low-level
ordering and lack of selectivity (volatile is "always on").
> I have no idea why he "mumbled" about it,
> the situation is pretty clear. Maybe your ignorance irritated him
> into a terse oneliner.
I don't think so. In the end he agreed that in the limited context we
were talking about at the time, volatile would have been okay. I decided
to change it anyway. If you're interested, the non-volatile version of
that code lives on today in uhci-hcd.h:td_status(). The entire problem
revolved around defeating an incorrect reordering by the compiler. And in
this case the "other processor" was a UHCI USB controller.
Alan Stern
-------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc. Do you grep through log files
for problems? Stop! Download the new AJAX search engine that makes
searching your log files as easy as surfing the web. DOWNLOAD SPLUNK!
http://ads.osdn.com/?ad_id=7637&alloc_id=16865&op=click
_______________________________________________
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
