Pete, Greg,
Again thank you very much for your valuable feedback. I generated another patch (based on linux-2.6.37-rc7) and ran it against scripts/checkpatch.pl script which didn't produce any errors or warnings:
[vtsozik@SR71 2.6.37-rc7]$ /usr/src/kernels/2.6.37-rc7/scripts/checkpatch.pl mct_u232.c.patch.submit
total: 0 errors, 0 warnings, 191 lines checked
mct_u232.c.patch.submit has no obvious style problems and is ready for submission.
[vtsozik@SR71 2.6.37-rc7]$
I attached mct_u232.c.patch.submit file to this email, so tabs will not be expanded into spaces. Please let me know if there's anything else I need to correct before it can be rolled out,
Again thank you for your help with this,
Vadim.
On Sun, 26 Dec 2010 18:58:46 -0800 (PST)
Tsozik <[email protected]> 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
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 <[email protected]> wrote:
> From: Pete Zaitcev <[email protected]>
> Subject: Re: [PATCH 1/1] mct_u232: added _ioctl and _get_icount functions
> To: "Tsozik" <[email protected]>
> Cc: "Greg Kroah-Hartman" <[email protected]>, [email protected], [email protected], [email protected]
> Date: Sunday, December 26, 2010, 10:57 PM
> On Sun, 26 Dec 2010 18:58:46 -0800
> (PST)
> Tsozik <[email protected]>
> 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
>
On Sun, 26 Dec 2010 22:34:23 -0800 (PST)
Tsozik <[email protected]> wrote:
> 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.
Sure, I know how typical UART works. Does the MCT device deliver the
change states? If it does and you want to use those, go ahead.
> 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.
Here's drivers/usb/serial/io_edgeport.c:
static void handle_new_msr(struct edgeport_port *edge_port, __u8 newMsr)
{
struct async_icount *icount;
dbg("%s %02x", __func__, newMsr);
if (newMsr & (EDGEPORT_MSR_DELTA_CTS | EDGEPORT_MSR_DELTA_DSR |
EDGEPORT_MSR_DELTA_RI | EDGEPORT_MSR_DELTA_CD)) {
icount = &edge_port->icount;
/* update input line counters */
if (newMsr & EDGEPORT_MSR_DELTA_CTS)
icount->cts++;
if (newMsr & EDGEPORT_MSR_DELTA_DSR)
icount->dsr++;
if (newMsr & EDGEPORT_MSR_DELTA_CD)
icount->dcd++;
if (newMsr & EDGEPORT_MSR_DELTA_RI)
icount->rng++;
wake_up_interruptible(&edge_port->delta_msr_wait);
}
Looks like it counts changes to me. And in any case, the gold standard
is in drivers/serial/8250.c:
static unsigned int check_modem_status(struct uart_8250_port *up)
{
unsigned int status = serial_in(up, UART_MSR);
status |= up->msr_saved_flags;
up->msr_saved_flags = 0;
if (status & UART_MSR_ANY_DELTA && up->ier & UART_IER_MSI &&
up->port.state != NULL) {
if (status & UART_MSR_TERI)
up->port.icount.rng++;
if (status & UART_MSR_DDSR)
up->port.icount.dsr++;
if (status & UART_MSR_DDCD)
uart_handle_dcd_change(&up->port, status & UART_MSR_DCD);
if (status & UART_MSR_DCTS)
uart_handle_cts_change(&up->port, status & UART_MSR_CTS);
wake_up_interruptible(&up->port.state->port.delta_msr_wait);
}
I only proposed accomplishing the same result by comparing states
in place of relying on hardware change reports like both of the
above do.
-- Pete