Date:	Sun, 26 Dec 2010 22:34:23 -0800 (PST)
From:	Tsozik <tsozik@...oo.com>
To:	Pete Zaitcev <zaitcev@...hat.com>
Cc:	Greg Kroah-Hartman <gregkh@...e.de>, linux-usb@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/1] mct_u232: added _ioctl and _get_icount functions

Pete,

Again many thanks for the additional review and comments. 

I addressed your second concern and added 3 locks/unlocks around places where mct_u232_port->icount struct is being read (I attached new patch to this email).

Regarding your first concern I have a slight reservation. Usually MSR contains information about both states which are raised and states which are changed. It looks like we use upper MSR nibble which contains information about 4 states which were raised, not lower MSR nibble which contains information about 4 states which were changed (Please refer to http://www.lammertbies.nl/comm/info/serial-uart.html for further details). So we set control state when it's raised and clear all the states which are not raised at the moment. Implementing your proposal would increase ring state reading by two times if between 2 consecutive rings another state is raised at the moment when first ring state is off. I also researched implementations under usb/serial and saw that all of them are rely on the current states (not change states) to increment the respective counter variables. In addition after I completed initial implementation I benchmarked rm60 radiation readings
 against PDM-2 readings. The differences were far below expected maximum expected error threshold.

Thank you again,
 Vadim.

--- On Sun, 12/26/10, Pete Zaitcev <zaitcev@...hat.com> wrote:

> From: Pete Zaitcev <zaitcev@...hat.com>
> Subject: Re: [PATCH 1/1] mct_u232: added _ioctl and _get_icount functions
> To: "Tsozik" <tsozik@...oo.com>
> Cc: "Greg Kroah-Hartman" <gregkh@...e.de>, linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org, zaitcev@...hat.com
> Date: Sunday, December 26, 2010, 10:57 PM
> On Sun, 26 Dec 2010 18:58:46 -0800
> (PST)
> Tsozik <tsozik@...oo.com>
> wrote:
> 
> > Please let me know if there's anything else I need to
> correct before
> > it can be rolled out,
> 
> I am hardly an expert here, but I looked it up as much as I
> could,
> and this looks like a wrong way to do it:
> 
> > @@ -386,27 +396,41 @@ static int
> mct_u232_get_modem_stat(struc
> >      /* Translate Control Line
> states */
> > +    if (msr & MCT_U232_MSR_DSR) {
> >         
> *control_state |=  TIOCM_DSR;
> > +       
> icount->dsr++;
> > +    } else {
> >         
> *control_state &= ~TIOCM_DSR;
> > +    }
> 
> Unfortunately, Kerrisk's manpages do not seem to offer an
> authoritative
> definition, but the expectations across the code seems that
> the counter
> actually counts the changes. I know that some people talked
> about
> counting "interrupts", but the way it's implemented in
> trustworthy
> drivers suggests something like this:
> 
>     unsigned int old_ctl_state;
> 
>     old_ctl_state = *control_state;
>     if (msr & MCT_U232_MSR_DSR)
>         *control_state
> |=  TIOCM_DSR;
>     else
>         *control_state &=
> ~TIOCM_DSR;
>     if (old_ctl_state != *control_state)
>         icount->dsr++;
>     ...... repeat for all bits
> 
> E.g. for DSR going down too, although perhaps I'm not
> suggesting the
> best way to do it. Could you re-implement it like the above
> and
> check that your radiation counter works the same with it
> and
> when driven by a built-in serial port?
> 
> Another thing, you should take priv->lock around
> fetching of
> mct_u232_port->icount in mct_u232_ioctl. It seems
> superfluous,
> I know, because the API itself is racy, but just to be
> nice
> and doing the expected thing...
> 
> -- Pete
>


      
Download attachment "mct_u232.c.patch.submit_v2" of type "application/octet-stream" (7161 bytes)

Powered by blists - more mailing lists