Re: [PATCH 2/2] tty: serial: msm: Support sysrq on uartDM devices
On 11/3/2014 2:05 AM, Daniel Thompson wrote:
> On 31/10/14 18:08, Frank Rowand wrote:
>> On 10/31/2014 2:43 AM, Daniel Thompson wrote:
>>> On 31/10/14 06:41, Stephen Boyd wrote:
On 10/30, Daniel Thompson wrote:
> On 29/10/14 18:14, Stephen Boyd wrote:
>> +r_count = min_t(int, count, sizeof(buf));
>> +
>> +for (i = 0; i < r_count; i++) {
>> +char flag = TTY_NORMAL;
>>
>> -/* TODO: handle sysrq */
>> -tty_insert_flip_string(tport, buf, min(count, 4));
>> -count -= 4;
>> +if (msm_port->break_detected && buf[i] == 0) {
>> +port->icount.brk++;
>> +flag = TTY_BREAK;
>> +msm_port->break_detected = false;
>> +if (uart_handle_break(port))
>> +continue;
>> +}
>> +
>> +if (!(port->read_status_mask &
>> UART_SR_RX_BREAK))
>> +flag = TTY_NORMAL;
>
> flag is already known to be TTY_NORMAL.
Huh? If we detected a break we would set the flag to TTY_BREAK
and if uart_handle_break() returned 0 (perhaps sysrq config is
diasbled) then we would get down here, and then we want to reset
the flag to TTY_NORMAL if the read_status_mask bits indicate that
we want to skip checking for breaks. Otherwise we want to
indicate to the tty layer that it's a break character.
>>>
>>> Agreed. Sorry for noise.
>>>
>>> It now reaches the level of silly quibble (meaning I won't bother to
>>> raise the issue again if there is a v2 patch) but perhaps updating the
>>> flag after the continue would be easier to read.
>>>
>>>
>> +
>> +spin_unlock(>lock);
>
> Is it safe to unlock at this point? count may no longer be valid when we
> return.
Can you explain further? If it actually isn't valid something
needs to be done. I believe other serial drivers are doing this
sort of thing though so it doesn't seem that uncommon (of course
those drivers could also be broken I suppose).
>>>
>>> Calling spin_unlock() means we are allow code to alter the state of the
>>> UART. In particular the subsequent call to uart_handle_sysrq_char() can
>>> make significant changes to the FIFO state (by calling the poll_char
>>> functions). Given count is shadowing the FIFO state, when we retake the
>>> lock I think it is possible for count to no longer be valid.
>>
>> uart_handle_sysrq_char() will not _read_ from the serial port. So it will
>> not directly modify the FIFO state.
>
> poll_char does not read from the FIFO? Since when?
>
> SysRq-g will enter cause the system to enter kdb/kgdb from within
> uart_handle_sysrq_char().
Aarrgh. You are correct. I overlooked the obvious SysRq-g.
/me searches for paper bag.
-Frank
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] tty: serial: msm: Support sysrq on uartDM devices
On 31/10/14 18:08, Frank Rowand wrote:
> On 10/31/2014 2:43 AM, Daniel Thompson wrote:
>> On 31/10/14 06:41, Stephen Boyd wrote:
>>> On 10/30, Daniel Thompson wrote:
On 29/10/14 18:14, Stephen Boyd wrote:
> + r_count = min_t(int, count, sizeof(buf));
> +
> + for (i = 0; i < r_count; i++) {
> + char flag = TTY_NORMAL;
>
> - /* TODO: handle sysrq */
> - tty_insert_flip_string(tport, buf, min(count, 4));
> - count -= 4;
> + if (msm_port->break_detected && buf[i] == 0) {
> + port->icount.brk++;
> + flag = TTY_BREAK;
> + msm_port->break_detected = false;
> + if (uart_handle_break(port))
> + continue;
> + }
> +
> + if (!(port->read_status_mask & UART_SR_RX_BREAK))
> + flag = TTY_NORMAL;
flag is already known to be TTY_NORMAL.
>>>
>>> Huh? If we detected a break we would set the flag to TTY_BREAK
>>> and if uart_handle_break() returned 0 (perhaps sysrq config is
>>> diasbled) then we would get down here, and then we want to reset
>>> the flag to TTY_NORMAL if the read_status_mask bits indicate that
>>> we want to skip checking for breaks. Otherwise we want to
>>> indicate to the tty layer that it's a break character.
>>
>> Agreed. Sorry for noise.
>>
>> It now reaches the level of silly quibble (meaning I won't bother to
>> raise the issue again if there is a v2 patch) but perhaps updating the
>> flag after the continue would be easier to read.
>>
>>
> +
> + spin_unlock(>lock);
Is it safe to unlock at this point? count may no longer be valid when we
return.
>>>
>>> Can you explain further? If it actually isn't valid something
>>> needs to be done. I believe other serial drivers are doing this
>>> sort of thing though so it doesn't seem that uncommon (of course
>>> those drivers could also be broken I suppose).
>>
>> Calling spin_unlock() means we are allow code to alter the state of the
>> UART. In particular the subsequent call to uart_handle_sysrq_char() can
>> make significant changes to the FIFO state (by calling the poll_char
>> functions). Given count is shadowing the FIFO state, when we retake the
>> lock I think it is possible for count to no longer be valid.
>
> uart_handle_sysrq_char() will not _read_ from the serial port. So it will
> not directly modify the FIFO state.
poll_char does not read from the FIFO? Since when?
SysRq-g will enter cause the system to enter kdb/kgdb from within
uart_handle_sysrq_char().
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] tty: serial: msm: Support sysrq on uartDM devices
On 31/10/14 18:08, Frank Rowand wrote:
On 10/31/2014 2:43 AM, Daniel Thompson wrote:
On 31/10/14 06:41, Stephen Boyd wrote:
On 10/30, Daniel Thompson wrote:
On 29/10/14 18:14, Stephen Boyd wrote:
+ r_count = min_t(int, count, sizeof(buf));
+
+ for (i = 0; i r_count; i++) {
+ char flag = TTY_NORMAL;
- /* TODO: handle sysrq */
- tty_insert_flip_string(tport, buf, min(count, 4));
- count -= 4;
+ if (msm_port-break_detected buf[i] == 0) {
+ port-icount.brk++;
+ flag = TTY_BREAK;
+ msm_port-break_detected = false;
+ if (uart_handle_break(port))
+ continue;
+ }
+
+ if (!(port-read_status_mask UART_SR_RX_BREAK))
+ flag = TTY_NORMAL;
flag is already known to be TTY_NORMAL.
Huh? If we detected a break we would set the flag to TTY_BREAK
and if uart_handle_break() returned 0 (perhaps sysrq config is
diasbled) then we would get down here, and then we want to reset
the flag to TTY_NORMAL if the read_status_mask bits indicate that
we want to skip checking for breaks. Otherwise we want to
indicate to the tty layer that it's a break character.
Agreed. Sorry for noise.
It now reaches the level of silly quibble (meaning I won't bother to
raise the issue again if there is a v2 patch) but perhaps updating the
flag after the continue would be easier to read.
+
+ spin_unlock(port-lock);
Is it safe to unlock at this point? count may no longer be valid when we
return.
Can you explain further? If it actually isn't valid something
needs to be done. I believe other serial drivers are doing this
sort of thing though so it doesn't seem that uncommon (of course
those drivers could also be broken I suppose).
Calling spin_unlock() means we are allow code to alter the state of the
UART. In particular the subsequent call to uart_handle_sysrq_char() can
make significant changes to the FIFO state (by calling the poll_char
functions). Given count is shadowing the FIFO state, when we retake the
lock I think it is possible for count to no longer be valid.
uart_handle_sysrq_char() will not _read_ from the serial port. So it will
not directly modify the FIFO state.
poll_char does not read from the FIFO? Since when?
SysRq-g will enter cause the system to enter kdb/kgdb from within
uart_handle_sysrq_char().
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] tty: serial: msm: Support sysrq on uartDM devices
On 11/3/2014 2:05 AM, Daniel Thompson wrote:
On 31/10/14 18:08, Frank Rowand wrote:
On 10/31/2014 2:43 AM, Daniel Thompson wrote:
On 31/10/14 06:41, Stephen Boyd wrote:
On 10/30, Daniel Thompson wrote:
On 29/10/14 18:14, Stephen Boyd wrote:
+r_count = min_t(int, count, sizeof(buf));
+
+for (i = 0; i r_count; i++) {
+char flag = TTY_NORMAL;
-/* TODO: handle sysrq */
-tty_insert_flip_string(tport, buf, min(count, 4));
-count -= 4;
+if (msm_port-break_detected buf[i] == 0) {
+port-icount.brk++;
+flag = TTY_BREAK;
+msm_port-break_detected = false;
+if (uart_handle_break(port))
+continue;
+}
+
+if (!(port-read_status_mask
UART_SR_RX_BREAK))
+flag = TTY_NORMAL;
flag is already known to be TTY_NORMAL.
Huh? If we detected a break we would set the flag to TTY_BREAK
and if uart_handle_break() returned 0 (perhaps sysrq config is
diasbled) then we would get down here, and then we want to reset
the flag to TTY_NORMAL if the read_status_mask bits indicate that
we want to skip checking for breaks. Otherwise we want to
indicate to the tty layer that it's a break character.
Agreed. Sorry for noise.
It now reaches the level of silly quibble (meaning I won't bother to
raise the issue again if there is a v2 patch) but perhaps updating the
flag after the continue would be easier to read.
+
+spin_unlock(port-lock);
Is it safe to unlock at this point? count may no longer be valid when we
return.
Can you explain further? If it actually isn't valid something
needs to be done. I believe other serial drivers are doing this
sort of thing though so it doesn't seem that uncommon (of course
those drivers could also be broken I suppose).
Calling spin_unlock() means we are allow code to alter the state of the
UART. In particular the subsequent call to uart_handle_sysrq_char() can
make significant changes to the FIFO state (by calling the poll_char
functions). Given count is shadowing the FIFO state, when we retake the
lock I think it is possible for count to no longer be valid.
uart_handle_sysrq_char() will not _read_ from the serial port. So it will
not directly modify the FIFO state.
poll_char does not read from the FIFO? Since when?
SysRq-g will enter cause the system to enter kdb/kgdb from within
uart_handle_sysrq_char().
Aarrgh. You are correct. I overlooked the obvious SysRq-g.
/me searches for paper bag.
-Frank
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] tty: serial: msm: Support sysrq on uartDM devices
On 10/31/2014 2:43 AM, Daniel Thompson wrote:
> On 31/10/14 06:41, Stephen Boyd wrote:
>> On 10/30, Daniel Thompson wrote:
>>> On 29/10/14 18:14, Stephen Boyd wrote:
+ r_count = min_t(int, count, sizeof(buf));
+
+ for (i = 0; i < r_count; i++) {
+ char flag = TTY_NORMAL;
- /* TODO: handle sysrq */
- tty_insert_flip_string(tport, buf, min(count, 4));
- count -= 4;
+ if (msm_port->break_detected && buf[i] == 0) {
+ port->icount.brk++;
+ flag = TTY_BREAK;
+ msm_port->break_detected = false;
+ if (uart_handle_break(port))
+ continue;
+ }
+
+ if (!(port->read_status_mask & UART_SR_RX_BREAK))
+ flag = TTY_NORMAL;
>>>
>>> flag is already known to be TTY_NORMAL.
>>
>> Huh? If we detected a break we would set the flag to TTY_BREAK
>> and if uart_handle_break() returned 0 (perhaps sysrq config is
>> diasbled) then we would get down here, and then we want to reset
>> the flag to TTY_NORMAL if the read_status_mask bits indicate that
>> we want to skip checking for breaks. Otherwise we want to
>> indicate to the tty layer that it's a break character.
>
> Agreed. Sorry for noise.
>
> It now reaches the level of silly quibble (meaning I won't bother to
> raise the issue again if there is a v2 patch) but perhaps updating the
> flag after the continue would be easier to read.
>
>
+
+ spin_unlock(>lock);
>>>
>>> Is it safe to unlock at this point? count may no longer be valid when we
>>> return.
>>
>> Can you explain further? If it actually isn't valid something
>> needs to be done. I believe other serial drivers are doing this
>> sort of thing though so it doesn't seem that uncommon (of course
>> those drivers could also be broken I suppose).
>
> Calling spin_unlock() means we are allow code to alter the state of the
> UART. In particular the subsequent call to uart_handle_sysrq_char() can
> make significant changes to the FIFO state (by calling the poll_char
> functions). Given count is shadowing the FIFO state, when we retake the
> lock I think it is possible for count to no longer be valid.
uart_handle_sysrq_char() will not _read_ from the serial port. So it will
not directly modify the FIFO state.
>
>>
>>>
>>>
+ sysrq = uart_handle_sysrq_char(port, buf[i]);
+ spin_lock(>lock);
+ if (!sysrq)
+ tty_insert_flip_char(tport, buf[i], flag);
>>>
>>> flag has a constant value here.
>>>
>>
>
>
> ___
> linux-arm-kernel mailing list
> linux-arm-ker...@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] tty: serial: msm: Support sysrq on uartDM devices
On 31/10/14 06:41, Stephen Boyd wrote:
> On 10/30, Daniel Thompson wrote:
>> On 29/10/14 18:14, Stephen Boyd wrote:
>>> + r_count = min_t(int, count, sizeof(buf));
>>> +
>>> + for (i = 0; i < r_count; i++) {
>>> + char flag = TTY_NORMAL;
>>>
>>> - /* TODO: handle sysrq */
>>> - tty_insert_flip_string(tport, buf, min(count, 4));
>>> - count -= 4;
>>> + if (msm_port->break_detected && buf[i] == 0) {
>>> + port->icount.brk++;
>>> + flag = TTY_BREAK;
>>> + msm_port->break_detected = false;
>>> + if (uart_handle_break(port))
>>> + continue;
>>> + }
>>> +
>>> + if (!(port->read_status_mask & UART_SR_RX_BREAK))
>>> + flag = TTY_NORMAL;
>>
>> flag is already known to be TTY_NORMAL.
>
> Huh? If we detected a break we would set the flag to TTY_BREAK
> and if uart_handle_break() returned 0 (perhaps sysrq config is
> diasbled) then we would get down here, and then we want to reset
> the flag to TTY_NORMAL if the read_status_mask bits indicate that
> we want to skip checking for breaks. Otherwise we want to
> indicate to the tty layer that it's a break character.
Agreed. Sorry for noise.
It now reaches the level of silly quibble (meaning I won't bother to
raise the issue again if there is a v2 patch) but perhaps updating the
flag after the continue would be easier to read.
>>> +
>>> + spin_unlock(>lock);
>>
>> Is it safe to unlock at this point? count may no longer be valid when we
>> return.
>
> Can you explain further? If it actually isn't valid something
> needs to be done. I believe other serial drivers are doing this
> sort of thing though so it doesn't seem that uncommon (of course
> those drivers could also be broken I suppose).
Calling spin_unlock() means we are allow code to alter the state of the
UART. In particular the subsequent call to uart_handle_sysrq_char() can
make significant changes to the FIFO state (by calling the poll_char
functions). Given count is shadowing the FIFO state, when we retake the
lock I think it is possible for count to no longer be valid.
>
>>
>>
>>> + sysrq = uart_handle_sysrq_char(port, buf[i]);
>>> + spin_lock(>lock);
>>> + if (!sysrq)
>>> + tty_insert_flip_char(tport, buf[i], flag);
>>
>> flag has a constant value here.
>>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] tty: serial: msm: Support sysrq on uartDM devices
On 10/30, Daniel Thompson wrote:
> On 29/10/14 18:14, Stephen Boyd wrote:
> > + r_count = min_t(int, count, sizeof(buf));
> > +
> > + for (i = 0; i < r_count; i++) {
> > + char flag = TTY_NORMAL;
> >
> > - /* TODO: handle sysrq */
> > - tty_insert_flip_string(tport, buf, min(count, 4));
> > - count -= 4;
> > + if (msm_port->break_detected && buf[i] == 0) {
> > + port->icount.brk++;
> > + flag = TTY_BREAK;
> > + msm_port->break_detected = false;
> > + if (uart_handle_break(port))
> > + continue;
> > + }
> > +
> > + if (!(port->read_status_mask & UART_SR_RX_BREAK))
> > + flag = TTY_NORMAL;
>
> flag is already known to be TTY_NORMAL.
Huh? If we detected a break we would set the flag to TTY_BREAK
and if uart_handle_break() returned 0 (perhaps sysrq config is
diasbled) then we would get down here, and then we want to reset
the flag to TTY_NORMAL if the read_status_mask bits indicate that
we want to skip checking for breaks. Otherwise we want to
indicate to the tty layer that it's a break character.
>
>
> > +
> > + spin_unlock(>lock);
>
> Is it safe to unlock at this point? count may no longer be valid when we
> return.
Can you explain further? If it actually isn't valid something
needs to be done. I believe other serial drivers are doing this
sort of thing though so it doesn't seem that uncommon (of course
those drivers could also be broken I suppose).
>
>
> > + sysrq = uart_handle_sysrq_char(port, buf[i]);
> > + spin_lock(>lock);
> > + if (!sysrq)
> > + tty_insert_flip_char(tport, buf[i], flag);
>
> flag has a constant value here.
>
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] tty: serial: msm: Support sysrq on uartDM devices
On 10/30, Daniel Thompson wrote:
On 29/10/14 18:14, Stephen Boyd wrote:
+ r_count = min_t(int, count, sizeof(buf));
+
+ for (i = 0; i r_count; i++) {
+ char flag = TTY_NORMAL;
- /* TODO: handle sysrq */
- tty_insert_flip_string(tport, buf, min(count, 4));
- count -= 4;
+ if (msm_port-break_detected buf[i] == 0) {
+ port-icount.brk++;
+ flag = TTY_BREAK;
+ msm_port-break_detected = false;
+ if (uart_handle_break(port))
+ continue;
+ }
+
+ if (!(port-read_status_mask UART_SR_RX_BREAK))
+ flag = TTY_NORMAL;
flag is already known to be TTY_NORMAL.
Huh? If we detected a break we would set the flag to TTY_BREAK
and if uart_handle_break() returned 0 (perhaps sysrq config is
diasbled) then we would get down here, and then we want to reset
the flag to TTY_NORMAL if the read_status_mask bits indicate that
we want to skip checking for breaks. Otherwise we want to
indicate to the tty layer that it's a break character.
+
+ spin_unlock(port-lock);
Is it safe to unlock at this point? count may no longer be valid when we
return.
Can you explain further? If it actually isn't valid something
needs to be done. I believe other serial drivers are doing this
sort of thing though so it doesn't seem that uncommon (of course
those drivers could also be broken I suppose).
+ sysrq = uart_handle_sysrq_char(port, buf[i]);
+ spin_lock(port-lock);
+ if (!sysrq)
+ tty_insert_flip_char(tport, buf[i], flag);
flag has a constant value here.
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] tty: serial: msm: Support sysrq on uartDM devices
On 31/10/14 06:41, Stephen Boyd wrote:
On 10/30, Daniel Thompson wrote:
On 29/10/14 18:14, Stephen Boyd wrote:
+ r_count = min_t(int, count, sizeof(buf));
+
+ for (i = 0; i r_count; i++) {
+ char flag = TTY_NORMAL;
- /* TODO: handle sysrq */
- tty_insert_flip_string(tport, buf, min(count, 4));
- count -= 4;
+ if (msm_port-break_detected buf[i] == 0) {
+ port-icount.brk++;
+ flag = TTY_BREAK;
+ msm_port-break_detected = false;
+ if (uart_handle_break(port))
+ continue;
+ }
+
+ if (!(port-read_status_mask UART_SR_RX_BREAK))
+ flag = TTY_NORMAL;
flag is already known to be TTY_NORMAL.
Huh? If we detected a break we would set the flag to TTY_BREAK
and if uart_handle_break() returned 0 (perhaps sysrq config is
diasbled) then we would get down here, and then we want to reset
the flag to TTY_NORMAL if the read_status_mask bits indicate that
we want to skip checking for breaks. Otherwise we want to
indicate to the tty layer that it's a break character.
Agreed. Sorry for noise.
It now reaches the level of silly quibble (meaning I won't bother to
raise the issue again if there is a v2 patch) but perhaps updating the
flag after the continue would be easier to read.
+
+ spin_unlock(port-lock);
Is it safe to unlock at this point? count may no longer be valid when we
return.
Can you explain further? If it actually isn't valid something
needs to be done. I believe other serial drivers are doing this
sort of thing though so it doesn't seem that uncommon (of course
those drivers could also be broken I suppose).
Calling spin_unlock() means we are allow code to alter the state of the
UART. In particular the subsequent call to uart_handle_sysrq_char() can
make significant changes to the FIFO state (by calling the poll_char
functions). Given count is shadowing the FIFO state, when we retake the
lock I think it is possible for count to no longer be valid.
+ sysrq = uart_handle_sysrq_char(port, buf[i]);
+ spin_lock(port-lock);
+ if (!sysrq)
+ tty_insert_flip_char(tport, buf[i], flag);
flag has a constant value here.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] tty: serial: msm: Support sysrq on uartDM devices
On 10/31/2014 2:43 AM, Daniel Thompson wrote:
On 31/10/14 06:41, Stephen Boyd wrote:
On 10/30, Daniel Thompson wrote:
On 29/10/14 18:14, Stephen Boyd wrote:
+ r_count = min_t(int, count, sizeof(buf));
+
+ for (i = 0; i r_count; i++) {
+ char flag = TTY_NORMAL;
- /* TODO: handle sysrq */
- tty_insert_flip_string(tport, buf, min(count, 4));
- count -= 4;
+ if (msm_port-break_detected buf[i] == 0) {
+ port-icount.brk++;
+ flag = TTY_BREAK;
+ msm_port-break_detected = false;
+ if (uart_handle_break(port))
+ continue;
+ }
+
+ if (!(port-read_status_mask UART_SR_RX_BREAK))
+ flag = TTY_NORMAL;
flag is already known to be TTY_NORMAL.
Huh? If we detected a break we would set the flag to TTY_BREAK
and if uart_handle_break() returned 0 (perhaps sysrq config is
diasbled) then we would get down here, and then we want to reset
the flag to TTY_NORMAL if the read_status_mask bits indicate that
we want to skip checking for breaks. Otherwise we want to
indicate to the tty layer that it's a break character.
Agreed. Sorry for noise.
It now reaches the level of silly quibble (meaning I won't bother to
raise the issue again if there is a v2 patch) but perhaps updating the
flag after the continue would be easier to read.
+
+ spin_unlock(port-lock);
Is it safe to unlock at this point? count may no longer be valid when we
return.
Can you explain further? If it actually isn't valid something
needs to be done. I believe other serial drivers are doing this
sort of thing though so it doesn't seem that uncommon (of course
those drivers could also be broken I suppose).
Calling spin_unlock() means we are allow code to alter the state of the
UART. In particular the subsequent call to uart_handle_sysrq_char() can
make significant changes to the FIFO state (by calling the poll_char
functions). Given count is shadowing the FIFO state, when we retake the
lock I think it is possible for count to no longer be valid.
uart_handle_sysrq_char() will not _read_ from the serial port. So it will
not directly modify the FIFO state.
+ sysrq = uart_handle_sysrq_char(port, buf[i]);
+ spin_lock(port-lock);
+ if (!sysrq)
+ tty_insert_flip_char(tport, buf[i], flag);
flag has a constant value here.
___
linux-arm-kernel mailing list
linux-arm-ker...@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] tty: serial: msm: Support sysrq on uartDM devices
On 29/10/14 18:14, Stephen Boyd wrote:
> To properly support sysrq on uartDM hardware we need to properly
> handle break characters. With the DM hardware the fifo can pack 4
> characters at a time, where a break is indicated by an all zero
> byte. Unfortunately, we can't differentiate between an all zero
> byte for a break and an all zero byte of data, so try and do as
> best we can. First unmask the RX break start interrupt and record
> the interrupt when it arrives. Then while processing the fifo,
> detect the break by searching for an all zero character as long
> as we recently received an RX break start interrupt. This should
> make sysrq work fairly well.
>
> Cc: Frank Rowand
> Cc: Daniel Thompson
> Signed-off-by: Stephen Boyd
> ---
> drivers/tty/serial/msm_serial.c | 41
> +++--
> drivers/tty/serial/msm_serial.h | 2 ++
> 2 files changed, 33 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/tty/serial/msm_serial.c b/drivers/tty/serial/msm_serial.c
> index cedcc36762a2..d44c04976f7a 100644
> --- a/drivers/tty/serial/msm_serial.c
> +++ b/drivers/tty/serial/msm_serial.c
> @@ -54,6 +54,7 @@ struct msm_port {
> unsigned intimr;
> int is_uartdm;
> unsigned intold_snap_state;
> + boolbreak_detected;
> };
>
> static inline void wait_for_xmitr(struct uart_port *port)
> @@ -126,23 +127,38 @@ static void handle_rx_dm(struct uart_port *port,
> unsigned int misr)
>
> while (count > 0) {
> unsigned char buf[4];
> + int sysrq, r_count, i;
>
> sr = msm_read(port, UART_SR);
> if ((sr & UART_SR_RX_READY) == 0) {
> msm_port->old_snap_state -= count;
> break;
> }
> +
> ioread32_rep(port->membase + UARTDM_RF, buf, 1);
> - if (sr & UART_SR_RX_BREAK) {
> - port->icount.brk++;
> - if (uart_handle_break(port))
> - continue;
> - } else if (sr & UART_SR_PAR_FRAME_ERR)
> - port->icount.frame++;
> + r_count = min_t(int, count, sizeof(buf));
> +
> + for (i = 0; i < r_count; i++) {
> + char flag = TTY_NORMAL;
>
> - /* TODO: handle sysrq */
> - tty_insert_flip_string(tport, buf, min(count, 4));
> - count -= 4;
> + if (msm_port->break_detected && buf[i] == 0) {
> + port->icount.brk++;
> + flag = TTY_BREAK;
> + msm_port->break_detected = false;
> + if (uart_handle_break(port))
> + continue;
> + }
> +
> + if (!(port->read_status_mask & UART_SR_RX_BREAK))
> + flag = TTY_NORMAL;
flag is already known to be TTY_NORMAL.
> +
> + spin_unlock(>lock);
Is it safe to unlock at this point? count may no longer be valid when we
return.
> + sysrq = uart_handle_sysrq_char(port, buf[i]);
> + spin_lock(>lock);
> + if (!sysrq)
> + tty_insert_flip_char(tport, buf[i], flag);
flag has a constant value here.
> + }
> + count -= r_count;
> }
>
> spin_unlock(>lock);
> @@ -291,6 +307,11 @@ static irqreturn_t msm_irq(int irq, void *dev_id)
> misr = msm_read(port, UART_MISR);
> msm_write(port, 0, UART_IMR); /* disable interrupt */
>
> + if (misr & UART_IMR_RXBREAK_START) {
> + msm_port->break_detected = true;
> + msm_write(port, UART_CR_CMD_RESET_RXBREAK_START, UART_CR);
> + }
> +
> if (misr & (UART_IMR_RXLEV | UART_IMR_RXSTALE)) {
> if (msm_port->is_uartdm)
> handle_rx_dm(port, misr);
> @@ -496,7 +517,7 @@ static int msm_startup(struct uart_port *port)
>
> /* turn on RX and CTS interrupts */
> msm_port->imr = UART_IMR_RXLEV | UART_IMR_RXSTALE |
> - UART_IMR_CURRENT_CTS;
> + UART_IMR_CURRENT_CTS | UART_IMR_RXBREAK_START;
>
> if (msm_port->is_uartdm) {
> msm_write(port, 0xFF, UARTDM_DMRX);
> diff --git a/drivers/tty/serial/msm_serial.h b/drivers/tty/serial/msm_serial.h
> index 73d3abe71e79..3e1c7138d8cd 100644
> --- a/drivers/tty/serial/msm_serial.h
> +++ b/drivers/tty/serial/msm_serial.h
> @@ -65,6 +65,7 @@
> #define UART_CR_TX_ENABLE(1 << 2)
> #define UART_CR_RX_DISABLE (1 << 1)
> #define UART_CR_RX_ENABLE(1 << 0)
> +#define UART_CR_CMD_RESET_RXBREAK_START ((1 << 11) | (2 << 4))
>
> #define UART_IMR 0x0014
> #define UART_IMR_TXLEV (1 << 0)
> @@ -72,6 +73,7 @@
> #define
Re: [PATCH 2/2] tty: serial: msm: Support sysrq on uartDM devices
On 29/10/14 18:14, Stephen Boyd wrote:
To properly support sysrq on uartDM hardware we need to properly
handle break characters. With the DM hardware the fifo can pack 4
characters at a time, where a break is indicated by an all zero
byte. Unfortunately, we can't differentiate between an all zero
byte for a break and an all zero byte of data, so try and do as
best we can. First unmask the RX break start interrupt and record
the interrupt when it arrives. Then while processing the fifo,
detect the break by searching for an all zero character as long
as we recently received an RX break start interrupt. This should
make sysrq work fairly well.
Cc: Frank Rowand frank.row...@sonymobile.com
Cc: Daniel Thompson daniel.thomp...@linaro.org
Signed-off-by: Stephen Boyd sb...@codeaurora.org
---
drivers/tty/serial/msm_serial.c | 41
+++--
drivers/tty/serial/msm_serial.h | 2 ++
2 files changed, 33 insertions(+), 10 deletions(-)
diff --git a/drivers/tty/serial/msm_serial.c b/drivers/tty/serial/msm_serial.c
index cedcc36762a2..d44c04976f7a 100644
--- a/drivers/tty/serial/msm_serial.c
+++ b/drivers/tty/serial/msm_serial.c
@@ -54,6 +54,7 @@ struct msm_port {
unsigned intimr;
int is_uartdm;
unsigned intold_snap_state;
+ boolbreak_detected;
};
static inline void wait_for_xmitr(struct uart_port *port)
@@ -126,23 +127,38 @@ static void handle_rx_dm(struct uart_port *port,
unsigned int misr)
while (count 0) {
unsigned char buf[4];
+ int sysrq, r_count, i;
sr = msm_read(port, UART_SR);
if ((sr UART_SR_RX_READY) == 0) {
msm_port-old_snap_state -= count;
break;
}
+
ioread32_rep(port-membase + UARTDM_RF, buf, 1);
- if (sr UART_SR_RX_BREAK) {
- port-icount.brk++;
- if (uart_handle_break(port))
- continue;
- } else if (sr UART_SR_PAR_FRAME_ERR)
- port-icount.frame++;
+ r_count = min_t(int, count, sizeof(buf));
+
+ for (i = 0; i r_count; i++) {
+ char flag = TTY_NORMAL;
- /* TODO: handle sysrq */
- tty_insert_flip_string(tport, buf, min(count, 4));
- count -= 4;
+ if (msm_port-break_detected buf[i] == 0) {
+ port-icount.brk++;
+ flag = TTY_BREAK;
+ msm_port-break_detected = false;
+ if (uart_handle_break(port))
+ continue;
+ }
+
+ if (!(port-read_status_mask UART_SR_RX_BREAK))
+ flag = TTY_NORMAL;
flag is already known to be TTY_NORMAL.
+
+ spin_unlock(port-lock);
Is it safe to unlock at this point? count may no longer be valid when we
return.
+ sysrq = uart_handle_sysrq_char(port, buf[i]);
+ spin_lock(port-lock);
+ if (!sysrq)
+ tty_insert_flip_char(tport, buf[i], flag);
flag has a constant value here.
+ }
+ count -= r_count;
}
spin_unlock(port-lock);
@@ -291,6 +307,11 @@ static irqreturn_t msm_irq(int irq, void *dev_id)
misr = msm_read(port, UART_MISR);
msm_write(port, 0, UART_IMR); /* disable interrupt */
+ if (misr UART_IMR_RXBREAK_START) {
+ msm_port-break_detected = true;
+ msm_write(port, UART_CR_CMD_RESET_RXBREAK_START, UART_CR);
+ }
+
if (misr (UART_IMR_RXLEV | UART_IMR_RXSTALE)) {
if (msm_port-is_uartdm)
handle_rx_dm(port, misr);
@@ -496,7 +517,7 @@ static int msm_startup(struct uart_port *port)
/* turn on RX and CTS interrupts */
msm_port-imr = UART_IMR_RXLEV | UART_IMR_RXSTALE |
- UART_IMR_CURRENT_CTS;
+ UART_IMR_CURRENT_CTS | UART_IMR_RXBREAK_START;
if (msm_port-is_uartdm) {
msm_write(port, 0xFF, UARTDM_DMRX);
diff --git a/drivers/tty/serial/msm_serial.h b/drivers/tty/serial/msm_serial.h
index 73d3abe71e79..3e1c7138d8cd 100644
--- a/drivers/tty/serial/msm_serial.h
+++ b/drivers/tty/serial/msm_serial.h
@@ -65,6 +65,7 @@
#define UART_CR_TX_ENABLE(1 2)
#define UART_CR_RX_DISABLE (1 1)
#define UART_CR_RX_ENABLE(1 0)
+#define UART_CR_CMD_RESET_RXBREAK_START ((1 11) | (2 4))
#define UART_IMR 0x0014
#define UART_IMR_TXLEV (1 0)
@@ -72,6 +73,7 @@
#define UART_IMR_RXLEV (1 4)
#define UART_IMR_DELTA_CTS (1 5)
[PATCH 2/2] tty: serial: msm: Support sysrq on uartDM devices
To properly support sysrq on uartDM hardware we need to properly
handle break characters. With the DM hardware the fifo can pack 4
characters at a time, where a break is indicated by an all zero
byte. Unfortunately, we can't differentiate between an all zero
byte for a break and an all zero byte of data, so try and do as
best we can. First unmask the RX break start interrupt and record
the interrupt when it arrives. Then while processing the fifo,
detect the break by searching for an all zero character as long
as we recently received an RX break start interrupt. This should
make sysrq work fairly well.
Cc: Frank Rowand
Cc: Daniel Thompson
Signed-off-by: Stephen Boyd
---
drivers/tty/serial/msm_serial.c | 41 +++--
drivers/tty/serial/msm_serial.h | 2 ++
2 files changed, 33 insertions(+), 10 deletions(-)
diff --git a/drivers/tty/serial/msm_serial.c b/drivers/tty/serial/msm_serial.c
index cedcc36762a2..d44c04976f7a 100644
--- a/drivers/tty/serial/msm_serial.c
+++ b/drivers/tty/serial/msm_serial.c
@@ -54,6 +54,7 @@ struct msm_port {
unsigned intimr;
int is_uartdm;
unsigned intold_snap_state;
+ boolbreak_detected;
};
static inline void wait_for_xmitr(struct uart_port *port)
@@ -126,23 +127,38 @@ static void handle_rx_dm(struct uart_port *port, unsigned
int misr)
while (count > 0) {
unsigned char buf[4];
+ int sysrq, r_count, i;
sr = msm_read(port, UART_SR);
if ((sr & UART_SR_RX_READY) == 0) {
msm_port->old_snap_state -= count;
break;
}
+
ioread32_rep(port->membase + UARTDM_RF, buf, 1);
- if (sr & UART_SR_RX_BREAK) {
- port->icount.brk++;
- if (uart_handle_break(port))
- continue;
- } else if (sr & UART_SR_PAR_FRAME_ERR)
- port->icount.frame++;
+ r_count = min_t(int, count, sizeof(buf));
+
+ for (i = 0; i < r_count; i++) {
+ char flag = TTY_NORMAL;
- /* TODO: handle sysrq */
- tty_insert_flip_string(tport, buf, min(count, 4));
- count -= 4;
+ if (msm_port->break_detected && buf[i] == 0) {
+ port->icount.brk++;
+ flag = TTY_BREAK;
+ msm_port->break_detected = false;
+ if (uart_handle_break(port))
+ continue;
+ }
+
+ if (!(port->read_status_mask & UART_SR_RX_BREAK))
+ flag = TTY_NORMAL;
+
+ spin_unlock(>lock);
+ sysrq = uart_handle_sysrq_char(port, buf[i]);
+ spin_lock(>lock);
+ if (!sysrq)
+ tty_insert_flip_char(tport, buf[i], flag);
+ }
+ count -= r_count;
}
spin_unlock(>lock);
@@ -291,6 +307,11 @@ static irqreturn_t msm_irq(int irq, void *dev_id)
misr = msm_read(port, UART_MISR);
msm_write(port, 0, UART_IMR); /* disable interrupt */
+ if (misr & UART_IMR_RXBREAK_START) {
+ msm_port->break_detected = true;
+ msm_write(port, UART_CR_CMD_RESET_RXBREAK_START, UART_CR);
+ }
+
if (misr & (UART_IMR_RXLEV | UART_IMR_RXSTALE)) {
if (msm_port->is_uartdm)
handle_rx_dm(port, misr);
@@ -496,7 +517,7 @@ static int msm_startup(struct uart_port *port)
/* turn on RX and CTS interrupts */
msm_port->imr = UART_IMR_RXLEV | UART_IMR_RXSTALE |
- UART_IMR_CURRENT_CTS;
+ UART_IMR_CURRENT_CTS | UART_IMR_RXBREAK_START;
if (msm_port->is_uartdm) {
msm_write(port, 0xFF, UARTDM_DMRX);
diff --git a/drivers/tty/serial/msm_serial.h b/drivers/tty/serial/msm_serial.h
index 73d3abe71e79..3e1c7138d8cd 100644
--- a/drivers/tty/serial/msm_serial.h
+++ b/drivers/tty/serial/msm_serial.h
@@ -65,6 +65,7 @@
#define UART_CR_TX_ENABLE (1 << 2)
#define UART_CR_RX_DISABLE (1 << 1)
#define UART_CR_RX_ENABLE (1 << 0)
+#define UART_CR_CMD_RESET_RXBREAK_START((1 << 11) | (2 << 4))
#define UART_IMR 0x0014
#define UART_IMR_TXLEV (1 << 0)
@@ -72,6 +73,7 @@
#define UART_IMR_RXLEV (1 << 4)
#define UART_IMR_DELTA_CTS (1 << 5)
#define UART_IMR_CURRENT_CTS (1 << 6)
+#define UART_IMR_RXBREAK_START (1 << 10)
#define UART_IPR_RXSTALE_LAST 0x20
#define UART_IPR_STALE_LSB 0x1F
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
[PATCH 2/2] tty: serial: msm: Support sysrq on uartDM devices
To properly support sysrq on uartDM hardware we need to properly
handle break characters. With the DM hardware the fifo can pack 4
characters at a time, where a break is indicated by an all zero
byte. Unfortunately, we can't differentiate between an all zero
byte for a break and an all zero byte of data, so try and do as
best we can. First unmask the RX break start interrupt and record
the interrupt when it arrives. Then while processing the fifo,
detect the break by searching for an all zero character as long
as we recently received an RX break start interrupt. This should
make sysrq work fairly well.
Cc: Frank Rowand frank.row...@sonymobile.com
Cc: Daniel Thompson daniel.thomp...@linaro.org
Signed-off-by: Stephen Boyd sb...@codeaurora.org
---
drivers/tty/serial/msm_serial.c | 41 +++--
drivers/tty/serial/msm_serial.h | 2 ++
2 files changed, 33 insertions(+), 10 deletions(-)
diff --git a/drivers/tty/serial/msm_serial.c b/drivers/tty/serial/msm_serial.c
index cedcc36762a2..d44c04976f7a 100644
--- a/drivers/tty/serial/msm_serial.c
+++ b/drivers/tty/serial/msm_serial.c
@@ -54,6 +54,7 @@ struct msm_port {
unsigned intimr;
int is_uartdm;
unsigned intold_snap_state;
+ boolbreak_detected;
};
static inline void wait_for_xmitr(struct uart_port *port)
@@ -126,23 +127,38 @@ static void handle_rx_dm(struct uart_port *port, unsigned
int misr)
while (count 0) {
unsigned char buf[4];
+ int sysrq, r_count, i;
sr = msm_read(port, UART_SR);
if ((sr UART_SR_RX_READY) == 0) {
msm_port-old_snap_state -= count;
break;
}
+
ioread32_rep(port-membase + UARTDM_RF, buf, 1);
- if (sr UART_SR_RX_BREAK) {
- port-icount.brk++;
- if (uart_handle_break(port))
- continue;
- } else if (sr UART_SR_PAR_FRAME_ERR)
- port-icount.frame++;
+ r_count = min_t(int, count, sizeof(buf));
+
+ for (i = 0; i r_count; i++) {
+ char flag = TTY_NORMAL;
- /* TODO: handle sysrq */
- tty_insert_flip_string(tport, buf, min(count, 4));
- count -= 4;
+ if (msm_port-break_detected buf[i] == 0) {
+ port-icount.brk++;
+ flag = TTY_BREAK;
+ msm_port-break_detected = false;
+ if (uart_handle_break(port))
+ continue;
+ }
+
+ if (!(port-read_status_mask UART_SR_RX_BREAK))
+ flag = TTY_NORMAL;
+
+ spin_unlock(port-lock);
+ sysrq = uart_handle_sysrq_char(port, buf[i]);
+ spin_lock(port-lock);
+ if (!sysrq)
+ tty_insert_flip_char(tport, buf[i], flag);
+ }
+ count -= r_count;
}
spin_unlock(port-lock);
@@ -291,6 +307,11 @@ static irqreturn_t msm_irq(int irq, void *dev_id)
misr = msm_read(port, UART_MISR);
msm_write(port, 0, UART_IMR); /* disable interrupt */
+ if (misr UART_IMR_RXBREAK_START) {
+ msm_port-break_detected = true;
+ msm_write(port, UART_CR_CMD_RESET_RXBREAK_START, UART_CR);
+ }
+
if (misr (UART_IMR_RXLEV | UART_IMR_RXSTALE)) {
if (msm_port-is_uartdm)
handle_rx_dm(port, misr);
@@ -496,7 +517,7 @@ static int msm_startup(struct uart_port *port)
/* turn on RX and CTS interrupts */
msm_port-imr = UART_IMR_RXLEV | UART_IMR_RXSTALE |
- UART_IMR_CURRENT_CTS;
+ UART_IMR_CURRENT_CTS | UART_IMR_RXBREAK_START;
if (msm_port-is_uartdm) {
msm_write(port, 0xFF, UARTDM_DMRX);
diff --git a/drivers/tty/serial/msm_serial.h b/drivers/tty/serial/msm_serial.h
index 73d3abe71e79..3e1c7138d8cd 100644
--- a/drivers/tty/serial/msm_serial.h
+++ b/drivers/tty/serial/msm_serial.h
@@ -65,6 +65,7 @@
#define UART_CR_TX_ENABLE (1 2)
#define UART_CR_RX_DISABLE (1 1)
#define UART_CR_RX_ENABLE (1 0)
+#define UART_CR_CMD_RESET_RXBREAK_START((1 11) | (2 4))
#define UART_IMR 0x0014
#define UART_IMR_TXLEV (1 0)
@@ -72,6 +73,7 @@
#define UART_IMR_RXLEV (1 4)
#define UART_IMR_DELTA_CTS (1 5)
#define UART_IMR_CURRENT_CTS (1 6)
+#define UART_IMR_RXBREAK_START (1 10)
#define UART_IPR_RXSTALE_LAST 0x20
#define UART_IPR_STALE_LSB 0x1F
--
The Qualcomm Innovation
