[PATCH v2] staging: speakup: document sysfs attributes
Speakup exposes a set of sysfs attributes under /sys/accessibility/speakup/ for user-space to interact with and configure speakup's kernel modules. This patch describes those attributes. Some attributes either lack a description or contain incomplete description. They are marked wit TODO. Authored-by: Gregory Nowak Submitted-by: Okash Khawaja Signed-off-by: Okash Khawaja --- drivers/staging/speakup/sysfs-driver-speakup | 369 +++ 1 file changed, 369 insertions(+) create mode 100644 drivers/staging/speakup/sysfs-driver-speakup diff --git a/drivers/staging/speakup/sysfs-driver-speakup b/drivers/staging/speakup/sysfs-driver-speakup new file mode 100644 index ..be3f5d6962e9 --- /dev/null +++ b/drivers/staging/speakup/sysfs-driver-speakup @@ -0,0 +1,369 @@ +What: /sys/accessibility/speakup/attrib_bleep +KernelVersion: 2.6 +Contact: spea...@linux-speakup.org +Description: Beeps the PC speaker when there is an attribute change such as + foreground or background color when using speakup review + commands. One = on, zero = off. + +What: /sys/accessibility/speakup/bell_pos +KernelVersion: 2.6 +Contact: spea...@linux-speakup.org +Description: This works much like a typewriter bell. If for example 72 is + echoed to bell_pos, it will beep the PC speaker when typing on + a line past character 72. + +What: /sys/accessibility/speakup/bleeps +KernelVersion: 2.6 +Contact: spea...@linux-speakup.org +Description: This controls whether one hears beeps through the PC speaker + when using speakup's review commands. + TODO: what values does it accept? + +What: /sys/accessibility/speakup/bleep_time +KernelVersion: 2.6 +Contact: spea...@linux-speakup.org +Description: This controls the duration of the PC speaker beeps speakup + produces. + TODO: What are the units? Jiffies? + +What: /sys/accessibility/speakup/cursor_time +KernelVersion: 2.6 +Contact: spea...@linux-speakup.org +Description: This controls cursor delay when using arrow keys. When a + connection is very slow, with the default setting, when moving + with the arrows, or backspacing etc. speakup says the incorrect + characters. Set this to a higher value to adjust for the delay + and better synchronisation between cursor position and speech. + +What: /sys/accessibility/speakup/delimiters +KernelVersion: 2.6 +Contact: spea...@linux-speakup.org +Description: Delimit a word from speakup. + TODO: add more info + +What: /sys/accessibility/speakup/ex_num +KernelVersion: 2.6 +Contact: spea...@linux-speakup.org +Description: TODO: + +What: /sys/accessibility/speakup/key_echo +KernelVersion: 2.6 +Contact: spea...@linux-speakup.org +Description: Controls if speakup speaks keys when they are typed. One = on, + zero = off or don't echo keys. + +What: /sys/accessibility/speakup/keymap +KernelVersion: 2.6 +Contact: spea...@linux-speakup.org +Description: Speakup keymap remaps keys to Speakup functions. + It uses a binary + format. A special program called genmap is needed to compile a + textual keymap into the binary format which is then loaded into + /sys/accessibility/speakup/keymap. + +What: /sys/accessibility/speakup/no_interrupt +KernelVersion: 2.6 +Contact: spea...@linux-speakup.org +Description: Controls if typing interrupts output from speakup. With + no_interrupt set to zero, typing on the keyboard will interrupt + speakup if for example + the say screen command is used before the + entire screen is read. + With no_interrupt set to one, if the say + screen command is used, and one then types on the keyboard, + speakup will continue to say the whole screen regardless until + it finishes. + +What: /sys/accessibility/speakup/punc_all +KernelVersion: 2.6 +Contact: spea...@linux-speakup.org +Description: This is a list of all the punctuation speakup should speak when + punc_level is set to four. + +What: /sys/accessibility/speakup/punc_level +KernelVersion: 2.6 +Contact: spea...@linux-speakup.org +Description: Controls the level of punctuation spoken as the screen is + displayed, not reviewed. Levels range from zero no punctuation, + to four, all punctuation. One corresponds to punc_some, two + corresponds to punc_most, and three as well as four both + correspond to punc_all. Some hardware synthesizers may have + different levels each corresponding to three and four for + p
[PATCH] staging: speakup: document sysfs attributes
Speakup exposes a set of sysfs attributes under /sys/accessibility/speakup/ for user-space to interact with and configure speakup's kernel modules. This patch describes those attributes. Some attributes either lack a description or contain incomplete description. They are marked wit TODO. Authored-by: Gregory Nowak Submitted-by: Okash Khawaja --- drivers/staging/speakup/sysfs-driver-speakup | 369 +++ 1 file changed, 369 insertions(+) create mode 100644 drivers/staging/speakup/sysfs-driver-speakup diff --git a/drivers/staging/speakup/sysfs-driver-speakup b/drivers/staging/speakup/sysfs-driver-speakup new file mode 100644 index ..be3f5d6962e9 --- /dev/null +++ b/drivers/staging/speakup/sysfs-driver-speakup @@ -0,0 +1,369 @@ +What: /sys/accessibility/speakup/attrib_bleep +KernelVersion: 2.6 +Contact: spea...@linux-speakup.org +Description: Beeps the PC speaker when there is an attribute change such as + foreground or background color when using speakup review + commands. One = on, zero = off. + +What: /sys/accessibility/speakup/bell_pos +KernelVersion: 2.6 +Contact: spea...@linux-speakup.org +Description: This works much like a typewriter bell. If for example 72 is + echoed to bell_pos, it will beep the PC speaker when typing on + a line past character 72. + +What: /sys/accessibility/speakup/bleeps +KernelVersion: 2.6 +Contact: spea...@linux-speakup.org +Description: This controls whether one hears beeps through the PC speaker + when using speakup's review commands. + TODO: what values does it accept? + +What: /sys/accessibility/speakup/bleep_time +KernelVersion: 2.6 +Contact: spea...@linux-speakup.org +Description: This controls the duration of the PC speaker beeps speakup + produces. + TODO: What are the units? Jiffies? + +What: /sys/accessibility/speakup/cursor_time +KernelVersion: 2.6 +Contact: spea...@linux-speakup.org +Description: This controls cursor delay when using arrow keys. When a + connection is very slow, with the default setting, when moving + with the arrows, or backspacing etc. speakup says the incorrect + characters. Set this to a higher value to adjust for the delay + and better synchronisation between cursor position and speech. + +What: /sys/accessibility/speakup/delimiters +KernelVersion: 2.6 +Contact: spea...@linux-speakup.org +Description: Delimit a word from speakup. + TODO: add more info + +What: /sys/accessibility/speakup/ex_num +KernelVersion: 2.6 +Contact: spea...@linux-speakup.org +Description: TODO: + +What: /sys/accessibility/speakup/key_echo +KernelVersion: 2.6 +Contact: spea...@linux-speakup.org +Description: Controls if speakup speaks keys when they are typed. One = on, + zero = off or don't echo keys. + +What: /sys/accessibility/speakup/keymap +KernelVersion: 2.6 +Contact: spea...@linux-speakup.org +Description: Speakup keymap remaps keys to Speakup functions. + It uses a binary + format. A special program called genmap is needed to compile a + textual keymap into the binary format which is then loaded into + /sys/accessibility/speakup/keymap. + +What: /sys/accessibility/speakup/no_interrupt +KernelVersion: 2.6 +Contact: spea...@linux-speakup.org +Description: Controls if typing interrupts output from speakup. With + no_interrupt set to zero, typing on the keyboard will interrupt + speakup if for example + the say screen command is used before the + entire screen is read. + With no_interrupt set to one, if the say + screen command is used, and one then types on the keyboard, + speakup will continue to say the whole screen regardless until + it finishes. + +What: /sys/accessibility/speakup/punc_all +KernelVersion: 2.6 +Contact: spea...@linux-speakup.org +Description: This is a list of all the punctuation speakup should speak when + punc_level is set to four. + +What: /sys/accessibility/speakup/punc_level +KernelVersion: 2.6 +Contact: spea...@linux-speakup.org +Description: Controls the level of punctuation spoken as the screen is + displayed, not reviewed. Levels range from zero no punctuation, + to four, all punctuation. One corresponds to punc_some, two + corresponds to punc_most, and three as well as four both + correspond to punc_all. Some hardware synthesizers may have + different levels each corresponding to three and four for + punc_level. Also note that i
Re: [HELP REQUESTED from the community] Was: Staging status of speakup
On Fri, Sep 20, 2019 at 8:46 AM Greg Kroah-Hartman wrote: > > On Wed, Sep 18, 2019 at 01:30:33PM -0700, Gregory Nowak wrote: > > > Extra line between each attribute (before the "What:" line) would be > > > nice. > > > > In a previous post above, you wrote: > > On Mon, Sep 16, 2019 at 04:11:00PM +0200, Greg Kroah-Hartman wrote: > > > Anyway, please put the Description: lines without a blank after that, > > > with the description text starting on that same line. > > > > I understood that to mean that the description text should start on > > the same line, and the blank lines after the description text should > > be removed. I've put them back in. Someone more familiar with the > > speakup code will have to dig into it to resolve the TODO items I > > suppose. > > No problem, this looks good to me. If someone wants to turn this into a > patch adding it to the drivers/staging/speakup/ directory, I'll be glad > to take it and it will allow others to fill in the TODO entries easier. Thank you. I'll create a patch soon.
Re: Re: [HELP REQUESTED from the community] Was: Staging status of speakup
Ah it looks like the spaces after Description: need to be converted into tabs. Thanks, Okash On Tue, Sep 17, 2019 at 10:35 PM Okash Khawaja wrote: > > Hi Greg, > > You're right, I got none of those emails. Thanks. Is it all taken care of? > > Best regards, > Okash > > On Tue, Sep 17, 2019 at 4:56 AM Gregory Nowak wrote: > > > > Hi Okash, > > I just realized the below didn't go to you directly along with the > > other addresses. > > > > Greg > > > > > > - Forwarded message from Gregory Nowak - > > > > Date: Mon, 16 Sep 2019 15:38:48 -0700 > > From: Gregory Nowak > > To: Greg Kroah-Hartman > > Cc: de...@driverdev.osuosl.org, Simon Dickson , > > "Speakup is a screen review system for Linux." > > , linux-kernel@vger.kernel.org, John > > Covici > > > > Subject: Re: [HELP REQUESTED from the community] Was: Staging status of > > speakup > > > > On Mon, Sep 16, 2019 at 04:11:00PM +0200, Greg Kroah-Hartman wrote: > > > On Mon, Sep 16, 2019 at 03:47:28PM +0200, Samuel Thibault wrote: > > > > Okash Khawaja, le dim. 15 sept. 2019 19:41:30 +0100, a ecrit: > > > > > I have attached the descriptions. > > > > > > > > Attachment is missing :) > > > > > > I saw it :) > > > > > > Anyway, please put the Description: lines without a blank after that, > > > with the description text starting on that same line. > > > > > > thanks! > > > > > > greg k-h > > > > It's attached. Hope the indentation is OK. > > > > Greg > > > > > > -- > > web site: http://www.gregn.net > > gpg public key: http://www.gregn.net/pubkey.asc > > skype: gregn1 > > (authorization required, add me to your contacts list first) > > If we haven't been in touch before, e-mail me before adding me to your > > contacts. > > > > -- > > Free domains: http://www.eu.org/ or mail dns-mana...@eu.org > > > > What: /sys/accessibility/speakup/attrib_bleep > > KernelVersion: 2.6 > > Contact:spea...@linux-speakup.org > > Description: Beeps the PC speaker when there is an attribute change such as > > foreground or background color when using speakup review > > commands. One = on, zero = off. > > What: /sys/accessibility/speakup/bell_pos > > KernelVersion: 2.6 > > Contact:spea...@linux-speakup.org > > Description: This works much like a typewriter bell. If for example 72 is > > echoed to bell_pos, it will beep the PC speaker when typing > > on > > a line past character 72. > > What: /sys/accessibility/speakup/bleeps > > KernelVersion: 2.6 > > Contact:spea...@linux-speakup.org > > Description: This controls whether one hears beeps through the PC speaker > > when using speakup's review commands. > > TODO: what values does it accept? > > What: /sys/accessibility/speakup/bleep_time > > KernelVersion: 2.6 > > Contact:spea...@linux-speakup.org > > Description: This controls the duration of the PC speaker beeps speakup > > produces. > > TODO: What are the units? Jiffies? > > What: /sys/accessibility/speakup/cursor_time > > KernelVersion: 2.6 > > Contact:spea...@linux-speakup.org > > Description: This controls cursor delay when using arrow keys. When a > > connection is very slow, with the default setting, when > > moving > > with the arrows, or backspacing etc. speakup says the > > incorrect > > characters. Set this to a higher value to adjust for the > > delay > > and better synchronisation between cursor position and > > speech. > > What: /sys/accessibility/speakup/delimiters > > KernelVersion: 2.6 > > Contact:spea...@linux-speakup.org > > Description: Delimit a word from speakup. > > TODO: add more info > > What: /sys/accessibility/speakup/ex_num > > KernelVersion: 2.6 > > Contact:spea...@linux-speakup.org > > Description: TODO: > > What: /sys/accessibility/speakup/key_echo > > KernelVersion: 2.6 > > Contact:spea...@linux-speakup.org > > Description: Controls if speakup speaks keys when they are typed. One = on, > > zero = off
Re: [HELP REQUESTED from the community] Was: Staging status of speakup
On Sun, Sep 15, 2019 at 2:43 PM Greg Kroah-Hartman wrote: > > On Sat, Sep 14, 2019 at 10:08:35PM +0100, Okash Khawaja wrote: > > On Mon, Sep 9, 2019 at 3:55 AM Gregory Nowak wrote: > > > > > > On Sun, Sep 08, 2019 at 10:43:02AM +0100, Okash Khawaja wrote: > > > > Sorry, I have only now got round to working on this. It's not complete > > > > yet but I have assimilated the feedback and converted subjective > > > > phrases, like "I think..." into objective statements or put them in > > > > TODO: so that someone else may verify. I have attached it to this > > > > email. > > > > > > I think bleeps needs a TODO, since we don't know what values it accepts, > > > or > > > what difference those values make. Also, to keep things uniform, we > > > should replace my "don't know" for trigger_time with a TODO. Looks > > > good to me otherwise. Thanks. > > > > Great thanks. I have updated. > > > > I have two questions: > > > > 1. Is it okay for these descriptions to go inside > > Documentation/ABI/stable? They have been around since 2.6 (2010). Or > > would we prefer Documentation/ABI/testing/? > > stable is fine. > > thanks, > > greg k-h Thanks Samuel and Greg. I have attached the descriptions. There are still some files marked with TODO, whose descriptions are incomplete or missing. Thanks, Okash sysfs-driver-speakup Description: Binary data
Re: [HELP REQUESTED from the community] Was: Staging status of speakup
On Mon, Sep 9, 2019 at 3:55 AM Gregory Nowak wrote: > > On Sun, Sep 08, 2019 at 10:43:02AM +0100, Okash Khawaja wrote: > > Sorry, I have only now got round to working on this. It's not complete > > yet but I have assimilated the feedback and converted subjective > > phrases, like "I think..." into objective statements or put them in > > TODO: so that someone else may verify. I have attached it to this > > email. > > I think bleeps needs a TODO, since we don't know what values it accepts, or > what difference those values make. Also, to keep things uniform, we > should replace my "don't know" for trigger_time with a TODO. Looks > good to me otherwise. Thanks. Great thanks. I have updated. I have two questions: 1. Is it okay for these descriptions to go inside Documentation/ABI/stable? They have been around since 2.6 (2010). Or would we prefer Documentation/ABI/testing/? 2. We are still missing descriptions for i18n/ directory. I have added filenames below. can someone can add description please: What: /sys/accessibility/speakup/i18n/announcements KernelVersion: 2.6 Contact:spea...@linux-speakup.org Description: TODO What: /sys/accessibility/speakup/i18n/chartab KernelVersion: 2.6 Contact:spea...@linux-speakup.org Description: TODO What: /sys/accessibility/speakup/i18n/ctl_keys KernelVersion: 2.6 Contact:spea...@linux-speakup.org Description: TODO What: /sys/accessibility/speakup/i18n/function_names KernelVersion: 2.6 Contact:spea...@linux-speakup.org Description: TODO What: /sys/accessibility/speakup/i18n/states KernelVersion: 2.6 Contact:spea...@linux-speakup.org Description: TODO What: /sys/accessibility/speakup/i18n/characters KernelVersion: 2.6 Contact:spea...@linux-speakup.org Description: TODO What: /sys/accessibility/speakup/i18n/colors KernelVersion: 2.6 Contact:spea...@linux-speakup.org Description: TODO What: /sys/accessibility/speakup/i18n/formatted KernelVersion: 2.6 Contact:spea...@linux-speakup.org Description: TODO What: /sys/accessibility/speakup/i18n/key_names KernelVersion: 2.6 Contact:spea...@linux-speakup.org Description: TODO Thanks, Okash
Re: [HELP REQUESTED from the community] Was: Staging status of speakup
Sorry, I have only now got round to working on this. It's not complete yet but I have assimilated the feedback and converted subjective phrases, like "I think..." into objective statements or put them in TODO: so that someone else may verify. I have attached it to this email. Next step will be to convert the format to match Documentation/ABI/ requirements. Thanks, Okash On Wed, Aug 21, 2019 at 11:23 PM Gregory Nowak wrote: > > On Wed, Aug 21, 2019 at 09:39:25AM -0700, Okash Khawaja wrote: > > Hi Greg N, > > > > Would like to send this as a patch as Greg K-H suggested? If not, I > > can do that with your email in Authored-by: tag? > > > > Thanks, > > Okash > > Hi Okash and all, > feel free to submit the patch with my email in the Authored-by: > tag if that's OK. Thanks, and good luck on your presentation. > > Greg > > > -- > web site: http://www.gregn.net > gpg public key: http://www.gregn.net/pubkey.asc > skype: gregn1 > (authorization required, add me to your contacts list first) > If we haven't been in touch before, e-mail me before adding me to your > contacts. > > -- > Free domains: http://www.eu.org/ or mail dns-mana...@eu.org > ___ > Speakup mailing list > spea...@linux-speakup.org > http://linux-speakup.org/cgi-bin/mailman/listinfo/speakup attrib_bleep Beeps the PC speaker when there is an attribute change such as foreground or background color when using speakup review commands. One = on, zero = off. bell_pos This works much like a typewriter bell. If for example 72 is echoed to bell_pos, it will beep the PC speaker when typing on a line past character 72. bleeps This controls whether one hears beeps through the PC speaker when using speakup's review commands. bleep_time This controls the duration of the PC speaker beeps speakup produces. TODO: What are the units? Jiffies? cursor_time This controls cursor delay when using arrow keys. When a connection is very slow, with the default setting, when moving with the arrows, or backspacing etc. speakup says the incorrect characters. Set this to a higher value to adjust for the delay and better synchronisation between cursor position and speech. delimiters Delimit a word from speakup. TODO: add more info ex_num TODO: key_echo Controls if speakup speaks keys when they are typed. One = on, zero = off or don't echo keys. keymap Speakup keymap remaps keys to Speakup functins. It uses a binary format. A special program called genmap is needed to compile a textual keymap into the binary format which is then loaded into /sys/accessibility/speakup/keymap. no_interrupt Controls if typing interrupts output from speakup. With no_interrupt set to zero, typing on the keyboard will interrupt speakup if for example the say screen command is used before the entire screen is read. With no_interrupt set to one, if the say screen command is used, and one then types on the keyboard, speakup will continue to say the whole screen regardless until it finishes. punc_all This is a list of all the punctuation speakup should speak when punc_level is set to four. punc_level Controls the level of punctuation spoken as the screen is displayed, not reviewed. Levels range from zero no punctuation, to four, all punctuation. One corresponds to punc_some, two corresponds to punc_most, and three as well as four both correspond to punc_all. Some hardware synthesizers may have different levels each corresponding to three and four for punc_level. Also note that if punc_level is set to zero, and key_echo is set to one, typed punctuation is still spoken as it is typed. punc_most This is a list of all the punctuation speakup should speak when punc_level is set to two. punc_some This is a list of all the punctuation speakup should speak when punc_level is set to one. reading_punc Almost the same as punc_level, the differences being that reading_punc controls the level of punctuation when reviewing the screen with speakup's screen review commands. The other difference is that reading_punc set to three speaks punc_all, and reading_punc set to four speaks all punctuation, including spaces. repeats A list of characters speakup repeats. Normally, when there are more than three characters in a row, speakup just reads three of those characters. For example, ".." would be read as dot, dot, dot. If a . is added to the list of characters in repeats, ".." would be read as dot, dot, dot, times six. say_control If set to one, speakup speaks shift, alt and control when those keys are pressed. If say_control is set to zero, shift, ctrl, and alt are not spoken when they are pressed. say_word_ctl TODO: silent TODO: spell_delay This controls how fast a word is spelled when speakup's say word review command is pressed twice quickly to speak the current word bei
Re: [HELP REQUESTED from the community] Was: Staging status of speakup
On Thu, Jul 25, 2019 at 3:49 AM John Covici wrote: > > I think the program is genmap, I have it in my init sequence, but I > am not sure it does anything at this point. > > On Thu, 25 Jul 2019 00:04:07 -0400, > Chris Brannon wrote: > > > > Gregory Nowak writes: > > > > > keymap > > > I believe this is the currently active kernel keymap. I'm not sure of > > > the format, probably what dumpkeys(1) and showkey(1) use. Echoing > > > different values here should allow for remapping speakup's review > > > commands besides remapping the keyboard as a whole. > > > > AFAIK the Speakup keymap is just for remapping keys to Speakup > > functions. It's a binary format, not related to dumpkeys etc. You need > > a special program to compile a textual keymap into something that can be > > loaded into /sys/accessibility/speakup/keymap. I may have source for > > that lying around here somewhere. This is "here there be dragons" > > territory. I think the only specification of the format is in the > > source code. > > > > -- Chris > > ___ > > Speakup mailing list > > spea...@linux-speakup.org > > http://linux-speakup.org/cgi-bin/mailman/listinfo/speakup > > -- > Your life is like a penny. You're going to lose it. The question is: > How do > you spend it? > > John Covici wb2una > cov...@ccs.covici.com > ___ > Speakup mailing list > spea...@linux-speakup.org > http://linux-speakup.org/cgi-bin/mailman/listinfo/speakup Hi Greg N, Would like to send this as a patch as Greg K-H suggested? If not, I can do that with your email in Authored-by: tag? Thanks, Okash
Re: Staging status of speakup
On Fri, Jul 12, 2019 at 9:38 AM Greg Kroah-Hartman wrote: > > On Sun, Jul 07, 2019 at 08:57:10AM +0200, Greg Kroah-Hartman wrote: > > On Sat, Jul 06, 2019 at 08:08:57PM +0100, Okash Khawaja wrote: > > > On Fri, 15 Mar 2019 20:18:31 -0700 > > > Greg Kroah-Hartman wrote: > > > > > > > On Fri, Mar 15, 2019 at 01:01:27PM +, Okash Khawaja wrote: > > > > > Hi, > > > > > > > > > > We have made progress on the items in TODO file of speakup driver in > > > > > staging directory and wanted to get some clarity on the remaining > > > > > items. Below is a summary of status of each item along with the > > > > > quotes from TODO file. > > > > > > > > > > 1. "The first issue has to do with the way speakup communicates > > > > > with serial ports. Currently, we communicate directly with the > > > > > hardware ports. This however conflicts with the standard serial > > > > > port drivers, which poses various problems. This is also not > > > > > working for modern hardware such as PCI-based serial ports. Also, > > > > > there is not a way we can communicate with USB devices. The > > > > > current serial port handling code is in serialio.c in this > > > > > directory." > > > > > > > > > > Drivers for all external synths now use TTY to communcate with the > > > > > devices. Only ones still using direct communication with hardware > > > > > ports are internal synths: acntpc, decpc, dtlk and keypc. These are > > > > > typically ISA cards and generally hardware which is difficult to > > > > > make work. We can leave these in staging. > > > > > > > > Ok, that's fine. > > > > > > > > > 2. "Some places are currently using in_atomic() because speakup > > > > > functions are called in various contexts, and a couple of things > > > > > can't happen in these cases. Pushing work to some worker thread > > > > > would probably help, as was already done for the serial port > > > > > driving part." > > > > > > > > > > There aren't any uses of in_atomic anymore. Commit d7500135802c > > > > > "Staging: speakup: Move pasting into a work item" was the last one > > > > > that removed such uses. > > > > > > > > Great, let's remove that todo item then. > > > > > > > > > 3. "There is a duplication of the selection functions in > > > > > selections.c. These functions should get exported from > > > > > drivers/char/selection.c (clear_selection notably) and used from > > > > > there instead." > > > > > > > > > > This is yet to be done. I guess drivers/char/selection.c is now > > > > > under drivers/tty/vt/selection.c. > > > > > > > > Yes, someone should update the todo item :) > > > > > > > > > 4. "The kobjects may have to move to a more proper place in /sys.The > > > > > discussion on lkml resulted to putting speech synthesizers in the > > > > > "speech" class, and the speakup screen reader itself > > > > > into /sys/class/vtconsole/vtcon0/speakup, the nasty path being > > > > > handled by userland tools." > > > > > > > > > > Although this makes logical sense, the change will mean changing > > > > > interface with userspace and hence the user space tools. I tried to > > > > > search the lkml discussion but couldn't find it. It will be good to > > > > > know your thoughts on this. > > > > > > > > I don't remember, sorry. I can review the kobject/sysfs usage if you > > > > think it is "good enough" now and see if I find anything > > > > objectionable. > > > > > > > > > Finally there is an issue where text in output buffer sometimes gets > > > > > garbled on SMP systems, but we can continue working on it after the > > > > > driver is moved out of staging, if that's okay. Basically we need a > > > > > reproducer of this issue. > > > > > > > > > > In addition to above, there are likely code style issues which will > > > > > need to be fixed. > > > > > > > > > > We are very keen to get speakup out of stag
Re: Staging status of speakup
On Fri, 15 Mar 2019 20:18:31 -0700 Greg Kroah-Hartman wrote: > On Fri, Mar 15, 2019 at 01:01:27PM +0000, Okash Khawaja wrote: > > Hi, > > > > We have made progress on the items in TODO file of speakup driver in > > staging directory and wanted to get some clarity on the remaining > > items. Below is a summary of status of each item along with the > > quotes from TODO file. > > > > 1. "The first issue has to do with the way speakup communicates > > with serial ports. Currently, we communicate directly with the > > hardware ports. This however conflicts with the standard serial > > port drivers, which poses various problems. This is also not > > working for modern hardware such as PCI-based serial ports. Also, > > there is not a way we can communicate with USB devices. The > > current serial port handling code is in serialio.c in this > > directory." > > > > Drivers for all external synths now use TTY to communcate with the > > devices. Only ones still using direct communication with hardware > > ports are internal synths: acntpc, decpc, dtlk and keypc. These are > > typically ISA cards and generally hardware which is difficult to > > make work. We can leave these in staging. > > Ok, that's fine. > > > 2. "Some places are currently using in_atomic() because speakup > > functions are called in various contexts, and a couple of things > > can't happen in these cases. Pushing work to some worker thread > > would probably help, as was already done for the serial port > > driving part." > > > > There aren't any uses of in_atomic anymore. Commit d7500135802c > > "Staging: speakup: Move pasting into a work item" was the last one > > that removed such uses. > > Great, let's remove that todo item then. > > > 3. "There is a duplication of the selection functions in > > selections.c. These functions should get exported from > > drivers/char/selection.c (clear_selection notably) and used from > > there instead." > > > > This is yet to be done. I guess drivers/char/selection.c is now > > under drivers/tty/vt/selection.c. > > Yes, someone should update the todo item :) > > > 4. "The kobjects may have to move to a more proper place in /sys.The > > discussion on lkml resulted to putting speech synthesizers in the > > "speech" class, and the speakup screen reader itself > > into /sys/class/vtconsole/vtcon0/speakup, the nasty path being > > handled by userland tools." > > > > Although this makes logical sense, the change will mean changing > > interface with userspace and hence the user space tools. I tried to > > search the lkml discussion but couldn't find it. It will be good to > > know your thoughts on this. > > I don't remember, sorry. I can review the kobject/sysfs usage if you > think it is "good enough" now and see if I find anything > objectionable. > > > Finally there is an issue where text in output buffer sometimes gets > > garbled on SMP systems, but we can continue working on it after the > > driver is moved out of staging, if that's okay. Basically we need a > > reproducer of this issue. > > > > In addition to above, there are likely code style issues which will > > need to be fixed. > > > > We are very keen to get speakup out of staging both, for settling > > the driver but also for getting included in distros which build > > only the mainline drivers. > > That's great, I am glad to see this happen. How about work on the > selection thing and then I can review the kobject stuff in a few > weeks, and then we can start moving things for 5.2? Hi Greg, Apologies for the delay. I de-duplicated selection code in speakup to use code that's already in kernel (commit ids 496124e5e16e and 41f13084506a). Following items are what remain now: 1. moving kobjects location 2. fixing garbled text I couldn't replicate garbled text but Simon (also in CC list) is looking into it. Can you please advise on the way forward? Thanks, Okash
[PATCH v2 0/2] staging: speakup: factor out selection code
Hi, The v2 renames set_selection() and do_set_selection() to following more explicit names: set_selection_user() /* includes copying data from user space */ set_selection_kernel() /* no copying from user space */ The patches also update references to set_selection() to be set_selection_user(). Original intro: Speakup's selection functionality parallels that of drivers/tty/vt/selection.c. This patch set replaces speakup's implementation with calls to vt's selection code. This is one of the remaining items in our TODO file and it's needed for moving speakup out of staging. Please note that in speakup selection is set inside interrupt context of keyboard handler. Set selection code in vt happens in process context and hence expects ability to sleep. To address this, there were two options: farm out speakup's set selection into a work_struct thread, or create atomic version of vt's set_selection. These patches implement the former option. Here's a summary: Patch 1 re-arranges code in vt and exports some functions. Patch 2 replaces speakup's selection code with calls to vt's functions exported in patch 1. Thanks, Okash
[PATCH v2 2/2] staging: speakup: refactor to use existing code in vt
This patch replaces speakup's implementations with calls to functions
in tty/vt/selection.c. Those functions are:
cancel_selection()
set_selection_kernel()
paste_selection()
Currently setting selection is done in interrupt context. However,
set_selection_kernel() can sleep - for instance, it requires console_lock
which can sleep. Therefore we offload that work to a work_struct thread,
following the same pattern which was already set for paste_selection().
When setting selection, we also get a reference to tty and make sure to
release the reference before returning.
struct speakup_paste_work has been renamed to the more generic
speakup_selection_work because it is now used for both pasting as well
as setting selection. When paste work is cancelled, the code wasn't
setting tty to NULL. This patch also fixes that by setting tty to NULL
so that in case of failure we don't get EBUSY forever.
Signed-off-by: Okash Khawaja
Reviewed-by: Samuel Thibault
Tested-by: Gregory Nowak
---
drivers/staging/speakup/main.c | 1 +
drivers/staging/speakup/selection.c | 212 +++-
drivers/staging/speakup/speakup.h | 1 +
3 files changed, 88 insertions(+), 126 deletions(-)
diff --git a/drivers/staging/speakup/main.c b/drivers/staging/speakup/main.c
index b6a65b0..488f253 100644
--- a/drivers/staging/speakup/main.c
+++ b/drivers/staging/speakup/main.c
@@ -2319,6 +2319,7 @@ static void __exit speakup_exit(void)
unregister_keyboard_notifier(&keyboard_notifier_block);
unregister_vt_notifier(&vt_notifier_block);
speakup_unregister_devsynth();
+ speakup_cancel_selection();
speakup_cancel_paste();
del_timer_sync(&cursor_timer);
kthread_stop(speakup_task);
diff --git a/drivers/staging/speakup/selection.c
b/drivers/staging/speakup/selection.c
index 0ed1fef..a8b4d0c 100644
--- a/drivers/staging/speakup/selection.c
+++ b/drivers/staging/speakup/selection.c
@@ -9,178 +9,138 @@
#include
#include
#include
+#include
#include "speakup.h"
-/* -- cut and paste - */
-/* Don't take this from : 011-015 on the screen aren't spaces */
-#define ishardspace(c) ((c) == ' ')
-
unsigned short spk_xs, spk_ys, spk_xe, spk_ye; /* our region points */
-
-/* Variables for selection control. */
-/* must not be deallocated */
struct vc_data *spk_sel_cons;
-/* cleared by clear_selection */
-static int sel_start = -1;
-static int sel_end;
-static int sel_buffer_lth;
-static char *sel_buffer;
-static unsigned char sel_pos(int n)
-{
- return inverse_translate(spk_sel_cons,
- screen_glyph(spk_sel_cons, n), 0);
-}
+struct speakup_selection_work {
+ struct work_struct work;
+ struct tiocl_selection sel;
+ struct tty_struct *tty;
+};
void speakup_clear_selection(void)
{
- sel_start = -1;
+ console_lock();
+ clear_selection();
+ console_unlock();
}
-/* does screen address p correspond to character at LH/RH edge of screen? */
-static int atedge(const int p, int size_row)
+static void __speakup_set_selection(struct work_struct *work)
{
- return !(p % size_row) || !((p + 2) % size_row);
-}
+ struct speakup_selection_work *ssw =
+ container_of(work, struct speakup_selection_work, work);
-/* constrain v such that v <= u */
-static unsigned short limit(const unsigned short v, const unsigned short u)
-{
- return (v > u) ? u : v;
-}
+ struct tty_struct *tty;
+ struct tiocl_selection sel;
-int speakup_set_selection(struct tty_struct *tty)
-{
- int new_sel_start, new_sel_end;
- char *bp, *obp;
- int i, ps, pe;
- struct vc_data *vc = vc_cons[fg_console].d;
+ sel = ssw->sel;
- spk_xs = limit(spk_xs, vc->vc_cols - 1);
- spk_ys = limit(spk_ys, vc->vc_rows - 1);
- spk_xe = limit(spk_xe, vc->vc_cols - 1);
- spk_ye = limit(spk_ye, vc->vc_rows - 1);
- ps = spk_ys * vc->vc_size_row + (spk_xs << 1);
- pe = spk_ye * vc->vc_size_row + (spk_xe << 1);
+ /* this ensures we copy sel before releasing the lock below */
+ rmb();
- if (ps > pe)/* make sel_start <= sel_end */
- swap(ps, pe);
+ /* release the lock by setting tty of the struct to NULL */
+ tty = xchg(&ssw->tty, NULL);
if (spk_sel_cons != vc_cons[fg_console].d) {
- speakup_clear_selection();
spk_sel_cons = vc_cons[fg_console].d;
- dev_warn(tty->dev,
-"Selection: mark console not the same as cut\n");
- return -EINVAL;
+ pr_warn("Selection: mark console not the same as cut\n");
+ goto unref;
}
- new_sel_start = ps;
- new_sel_end = pe;
-
- /* select to end of line if on trailing space */
-
[PATCH v2 1/2] vt: selection: allow functions to be called from inside kernel
This patch breaks set_selection() into two functions so that when
called from kernel, copy_from_user() can be avoided. The two functions
are called set_selection_user() and set_selection_kernel() in order to
be explicit about their purposes. This also means updating any
references to set_selection() and fixing for name change. It also
exports set_selection_kernel() and paste_selection().
These changes are used the following patch where speakup's selection
functionality calls into the above functions, thereby doing away with
parallel implementation.
Signed-off-by: Okash Khawaja
Reviewed-by: Samuel Thibault
Tested-by: Gregory Nowak
---
drivers/tty/vt/selection.c | 46 ++
drivers/tty/vt/vt.c| 7 ---
include/linux/selection.h | 7 ---
3 files changed, 38 insertions(+), 22 deletions(-)
diff --git a/drivers/tty/vt/selection.c b/drivers/tty/vt/selection.c
index 07496c7..78732fe 100644
--- a/drivers/tty/vt/selection.c
+++ b/drivers/tty/vt/selection.c
@@ -2,7 +2,9 @@
/*
* This module exports the functions:
*
- * 'int set_selection(struct tiocl_selection __user *, struct tty_struct
*)'
+ * 'int set_selection_user(struct tiocl_selection __user *,
+ *struct tty_struct *)'
+ * 'int set_selection_kernel(struct tiocl_selection *, struct tty_struct
*)'
* 'void clear_selection(void)'
* 'int paste_selection(struct tty_struct *)'
* 'int sel_loadlut(char __user *)'
@@ -80,6 +82,7 @@ void clear_selection(void)
sel_start = -1;
}
}
+EXPORT_SYMBOL_GPL(clear_selection);
/*
* User settable table: what characters are to be considered alphabetic?
@@ -154,7 +157,7 @@ static int store_utf8(u32 c, char *p)
}
/**
- * set_selection - set the current selection.
+ * set_selection_user - set the current selection.
* @sel: user selection info
* @tty: the console tty
*
@@ -163,35 +166,44 @@ static int store_utf8(u32 c, char *p)
* The entire selection process is managed under the console_lock. It's
* a lot under the lock but its hardly a performance path
*/
-int set_selection(const struct tiocl_selection __user *sel, struct tty_struct
*tty)
+int set_selection_user(const struct tiocl_selection __user *sel,
+ struct tty_struct *tty)
+{
+ struct tiocl_selection v;
+
+ if (copy_from_user(&v, sel, sizeof(*sel)))
+ return -EFAULT;
+
+ return set_selection_kernel(&v, tty);
+}
+
+int set_selection_kernel(struct tiocl_selection *v, struct tty_struct *tty)
{
struct vc_data *vc = vc_cons[fg_console].d;
int new_sel_start, new_sel_end, spc;
- struct tiocl_selection v;
char *bp, *obp;
int i, ps, pe, multiplier;
u32 c;
int mode;
poke_blanked_console();
- if (copy_from_user(&v, sel, sizeof(*sel)))
- return -EFAULT;
- v.xs = min_t(u16, v.xs - 1, vc->vc_cols - 1);
- v.ys = min_t(u16, v.ys - 1, vc->vc_rows - 1);
- v.xe = min_t(u16, v.xe - 1, vc->vc_cols - 1);
- v.ye = min_t(u16, v.ye - 1, vc->vc_rows - 1);
- ps = v.ys * vc->vc_size_row + (v.xs << 1);
- pe = v.ye * vc->vc_size_row + (v.xe << 1);
+ v->xs = min_t(u16, v->xs - 1, vc->vc_cols - 1);
+ v->ys = min_t(u16, v->ys - 1, vc->vc_rows - 1);
+ v->xe = min_t(u16, v->xe - 1, vc->vc_cols - 1);
+ v->ye = min_t(u16, v->ye - 1, vc->vc_rows - 1);
+ ps = v->ys * vc->vc_size_row + (v->xs << 1);
+ pe = v->ye * vc->vc_size_row + (v->xe << 1);
- if (v.sel_mode == TIOCL_SELCLEAR) {
+ if (v->sel_mode == TIOCL_SELCLEAR) {
/* useful for screendump without selection highlights */
clear_selection();
return 0;
}
- if (mouse_reporting() && (v.sel_mode & TIOCL_SELMOUSEREPORT)) {
- mouse_report(tty, v.sel_mode & TIOCL_SELBUTTONMASK, v.xs, v.ys);
+ if (mouse_reporting() && (v->sel_mode & TIOCL_SELMOUSEREPORT)) {
+ mouse_report(tty, v->sel_mode & TIOCL_SELBUTTONMASK, v->xs,
+v->ys);
return 0;
}
@@ -208,7 +220,7 @@ int set_selection(const struct tiocl_selection __user *sel,
struct tty_struct *t
else
use_unicode = 0;
- switch (v.sel_mode)
+ switch (v->sel_mode)
{
case TIOCL_SELCHAR: /* character-by-character selection */
new_sel_start = ps;
@@ -322,6 +334,7 @@ int set_selection(const struct tiocl_selection __user *sel,
struct tty_struct *t
sel_buffer_lth = bp - sel_buffer;
return 0;
}
+EXPORT_SYMBOL_GPL(se
[PATCH 1/2] vt: selection: allow functions to be called from inside kernel
This patch breaks set_selection() into two functions so that when
called from kernel, copy_from_user() can be avoided. It also exports
set_selection() and paste_selection().
These changes are used the following patch where speakup's selection
functionality calls into the above functions, thereby doing away with
parallel implementation.
Signed-off-by: Okash Khawaja
Reviewed-by: Samuel Thibault
Tested-by: Gregory Nowak
---
drivers/tty/vt/selection.c | 37 -
include/linux/selection.h | 3 +--
2 files changed, 25 insertions(+), 15 deletions(-)
diff --git a/drivers/tty/vt/selection.c b/drivers/tty/vt/selection.c
index 07496c711d7d..a43f9cd9bdd6 100644
--- a/drivers/tty/vt/selection.c
+++ b/drivers/tty/vt/selection.c
@@ -80,6 +80,7 @@ void clear_selection(void)
sel_start = -1;
}
}
+EXPORT_SYMBOL_GPL(clear_selection);
/*
* User settable table: what characters are to be considered alphabetic?
@@ -164,34 +165,42 @@ static int store_utf8(u32 c, char *p)
* a lot under the lock but its hardly a performance path
*/
int set_selection(const struct tiocl_selection __user *sel, struct tty_struct
*tty)
+{
+ struct tiocl_selection v;
+
+ if (copy_from_user(&v, sel, sizeof(*sel)))
+ return -EFAULT;
+
+ return do_set_selection(&v, tty);
+}
+
+int do_set_selection(struct tiocl_selection *v, struct tty_struct *tty)
{
struct vc_data *vc = vc_cons[fg_console].d;
int new_sel_start, new_sel_end, spc;
- struct tiocl_selection v;
char *bp, *obp;
int i, ps, pe, multiplier;
u32 c;
int mode;
poke_blanked_console();
- if (copy_from_user(&v, sel, sizeof(*sel)))
- return -EFAULT;
- v.xs = min_t(u16, v.xs - 1, vc->vc_cols - 1);
- v.ys = min_t(u16, v.ys - 1, vc->vc_rows - 1);
- v.xe = min_t(u16, v.xe - 1, vc->vc_cols - 1);
- v.ye = min_t(u16, v.ye - 1, vc->vc_rows - 1);
- ps = v.ys * vc->vc_size_row + (v.xs << 1);
- pe = v.ye * vc->vc_size_row + (v.xe << 1);
+ v->xs = min_t(u16, v->xs - 1, vc->vc_cols - 1);
+ v->ys = min_t(u16, v->ys - 1, vc->vc_rows - 1);
+ v->xe = min_t(u16, v->xe - 1, vc->vc_cols - 1);
+ v->ye = min_t(u16, v->ye - 1, vc->vc_rows - 1);
+ ps = v->ys * vc->vc_size_row + (v->xs << 1);
+ pe = v->ye * vc->vc_size_row + (v->xe << 1);
- if (v.sel_mode == TIOCL_SELCLEAR) {
+ if (v->sel_mode == TIOCL_SELCLEAR) {
/* useful for screendump without selection highlights */
clear_selection();
return 0;
}
- if (mouse_reporting() && (v.sel_mode & TIOCL_SELMOUSEREPORT)) {
- mouse_report(tty, v.sel_mode & TIOCL_SELBUTTONMASK, v.xs, v.ys);
+ if (mouse_reporting() && (v->sel_mode & TIOCL_SELMOUSEREPORT)) {
+ mouse_report(tty, v->sel_mode & TIOCL_SELBUTTONMASK, v->xs,
+v->ys);
return 0;
}
@@ -208,7 +217,7 @@ int set_selection(const struct tiocl_selection __user *sel,
struct tty_struct *t
else
use_unicode = 0;
- switch (v.sel_mode)
+ switch (v->sel_mode)
{
case TIOCL_SELCHAR: /* character-by-character selection */
new_sel_start = ps;
@@ -322,6 +331,7 @@ int set_selection(const struct tiocl_selection __user *sel,
struct tty_struct *t
sel_buffer_lth = bp - sel_buffer;
return 0;
}
+EXPORT_SYMBOL_GPL(do_set_selection);
/* Insert the contents of the selection buffer into the
* queue of the tty associated with the current console.
@@ -367,3 +377,4 @@ int paste_selection(struct tty_struct *tty)
tty_ldisc_deref(ld);
return 0;
}
+EXPORT_SYMBOL_GPL(paste_selection);
diff --git a/include/linux/selection.h b/include/linux/selection.h
index a8f5b97b216f..171d77dfc825 100644
--- a/include/linux/selection.h
+++ b/include/linux/selection.h
@@ -11,13 +11,12 @@
#include
#include
-struct tty_struct;
-
extern struct vc_data *sel_cons;
struct tty_struct;
extern void clear_selection(void);
extern int set_selection(const struct tiocl_selection __user *sel, struct
tty_struct *tty);
+extern int do_set_selection(struct tiocl_selection *v, struct tty_struct *tty);
extern int paste_selection(struct tty_struct *tty);
extern int sel_loadlut(char __user *p);
extern int mouse_reporting(void);
--
2.21.0
[PATCH 2/2] staging: speakup: refactor to use existing code in vt
This patch replaces speakup's implementations with calls to functions
in tty/vt/selection.c. Those functions are:
cancel_selection()
do_set_selection()
paste_selection()
Currently setting selection is done in interrupt context. However,
do_set_selection() can sleep - for instance, it requires console_lock
which can sleep. Therefore we offload that work to a work_struct thread,
following the same pattern which was already set for paste_selection().
When setting selection, we also get a reference to tty and make sure to
release the reference before returning.
struct speakup_paste_work has been renamed to the more generic
speakup_selection_work because it is now used for both pasting as well
as setting selection. When paste work is cancelled, the code wasn't
setting tty to NULL. This patch also fixes that by setting tty to NULL
so that in case of failure we don't get EBUSY forever.
Signed-off-by: Okash Khawaja
Reviewed-by: Samuel Thibault
Tested-by: Gregory Nowak
---
drivers/staging/speakup/main.c | 1 +
drivers/staging/speakup/selection.c | 212 +++-
drivers/staging/speakup/speakup.h | 1 +
3 files changed, 88 insertions(+), 126 deletions(-)
diff --git a/drivers/staging/speakup/main.c b/drivers/staging/speakup/main.c
index b6a65b0c1896..488f2539aa9a 100644
--- a/drivers/staging/speakup/main.c
+++ b/drivers/staging/speakup/main.c
@@ -2319,6 +2319,7 @@ static void __exit speakup_exit(void)
unregister_keyboard_notifier(&keyboard_notifier_block);
unregister_vt_notifier(&vt_notifier_block);
speakup_unregister_devsynth();
+ speakup_cancel_selection();
speakup_cancel_paste();
del_timer_sync(&cursor_timer);
kthread_stop(speakup_task);
diff --git a/drivers/staging/speakup/selection.c
b/drivers/staging/speakup/selection.c
index 0ed1fefee0e9..6c6d77f8bc24 100644
--- a/drivers/staging/speakup/selection.c
+++ b/drivers/staging/speakup/selection.c
@@ -9,178 +9,138 @@
#include
#include
#include
+#include
#include "speakup.h"
-/* -- cut and paste - */
-/* Don't take this from : 011-015 on the screen aren't spaces */
-#define ishardspace(c) ((c) == ' ')
-
unsigned short spk_xs, spk_ys, spk_xe, spk_ye; /* our region points */
-
-/* Variables for selection control. */
-/* must not be deallocated */
struct vc_data *spk_sel_cons;
-/* cleared by clear_selection */
-static int sel_start = -1;
-static int sel_end;
-static int sel_buffer_lth;
-static char *sel_buffer;
-static unsigned char sel_pos(int n)
-{
- return inverse_translate(spk_sel_cons,
- screen_glyph(spk_sel_cons, n), 0);
-}
+struct speakup_selection_work {
+ struct work_struct work;
+ struct tiocl_selection sel;
+ struct tty_struct *tty;
+};
void speakup_clear_selection(void)
{
- sel_start = -1;
+ console_lock();
+ clear_selection();
+ console_unlock();
}
-/* does screen address p correspond to character at LH/RH edge of screen? */
-static int atedge(const int p, int size_row)
+static void __speakup_set_selection(struct work_struct *work)
{
- return !(p % size_row) || !((p + 2) % size_row);
-}
+ struct speakup_selection_work *ssw =
+ container_of(work, struct speakup_selection_work, work);
-/* constrain v such that v <= u */
-static unsigned short limit(const unsigned short v, const unsigned short u)
-{
- return (v > u) ? u : v;
-}
+ struct tty_struct *tty;
+ struct tiocl_selection sel;
-int speakup_set_selection(struct tty_struct *tty)
-{
- int new_sel_start, new_sel_end;
- char *bp, *obp;
- int i, ps, pe;
- struct vc_data *vc = vc_cons[fg_console].d;
+ sel = ssw->sel;
- spk_xs = limit(spk_xs, vc->vc_cols - 1);
- spk_ys = limit(spk_ys, vc->vc_rows - 1);
- spk_xe = limit(spk_xe, vc->vc_cols - 1);
- spk_ye = limit(spk_ye, vc->vc_rows - 1);
- ps = spk_ys * vc->vc_size_row + (spk_xs << 1);
- pe = spk_ye * vc->vc_size_row + (spk_xe << 1);
+ /* this ensures we copy sel before releasing the lock below */
+ rmb();
- if (ps > pe)/* make sel_start <= sel_end */
- swap(ps, pe);
+ /* release the lock by setting tty of the struct to NULL */
+ tty = xchg(&ssw->tty, NULL);
if (spk_sel_cons != vc_cons[fg_console].d) {
- speakup_clear_selection();
spk_sel_cons = vc_cons[fg_console].d;
- dev_warn(tty->dev,
-"Selection: mark console not the same as cut\n");
- return -EINVAL;
+ pr_warn("Selection: mark console not the same as cut\n");
+ goto unref;
}
- new_sel_start = ps;
- new_sel_end = pe;
-
- /* select to end of line if on trailing space */
-
[PATCH 0/2] staging: speakup: factor out selection code
Hi, Speakup's selection functionality parallels that of drivers/tty/vt/selection.c. This patch set replaces speakup's implementation with calls to vt's selection code. This is one of the remaining items in our TODO file and it's needed for moving speakup out of staging. Please note that in speakup selection is set inside interrupt context of keyboard handler. Set selection code in vt happens in process context and hence expects ability to sleep. To address this, there were two options: farm out speakup's set selection into a work_struct thread, or create atomic version of vt's set_selection. These patches implement the former option. Here's a summary: Patch 1 re-arranges code in vt and exports some functions. Patch 2 replaces speakup's selection code with calls to vt's functions exported in patch 1. Thanks, Okash
Re: Staging status of speakup
On Tue, 19 Mar 2019 16:31:21 + Alan Cox wrote: > On Sat, 16 Mar 2019 10:35:43 +0100 > Samuel Thibault wrote: > > > Chris Brannon, le ven. 15 mars 2019 18:19:39 -0700, a ecrit: > > > Okash Khawaja writes: > > > > Finally there is an issue where text in output buffer sometimes > > > > gets garbled on SMP systems, but we can continue working on it > > > > after the driver is moved out of staging, if that's okay. > > > > Basically we need a reproducer of this issue. > > > > > > What kind of reproducer do you need here? It's straightforward to > > > reproduce in casual use, at least with a software synthesizer. > > > > The problem is that neither Okash nor I are even casual users of > > speakup, so we need a walk-through of the kind of operation that > > produces the issue. It does not have to be reproducible each time > > it is done. Perhaps (I really don't know what that bug is about > > actually) it is a matter of putting text in the selection buffer, > > and try to paste it 100 times, and once every 10 times it will be > > garbled, for instance. > > paste_selection still says > > /* Insert the contents of the selection buffer into the > * queue of the tty associated with the current console. > * Invoked by ioctl(). > * > * Locking: called without locks. Calls the ldisc wrongly with > * unsafe methods, > */ > > from which I deduce that with everyone using X nobody ever bothered to > fix it. So before you look too hard at the speakup code you might > want to review the interaction with selection.c too. Hi, This is a good point. At the moment speakup uses its own set of selection and paste functions but I am in process of changing speakup to use these functions from drivers/tty/vt/selection.c instead. This lack of locking will be worth watching out for. Thanks! Okash
Re: Staging status of speakup
On Fri, 15 Mar 2019 20:18:31 -0700 Greg Kroah-Hartman wrote: > On Fri, Mar 15, 2019 at 01:01:27PM +0000, Okash Khawaja wrote: > > Hi, > > > > We have made progress on the items in TODO file of speakup driver in > > staging directory and wanted to get some clarity on the remaining > > items. Below is a summary of status of each item along with the > > quotes from TODO file. > > > > 1. "The first issue has to do with the way speakup communicates > > with serial ports. Currently, we communicate directly with the > > hardware ports. This however conflicts with the standard serial > > port drivers, which poses various problems. This is also not > > working for modern hardware such as PCI-based serial ports. Also, > > there is not a way we can communicate with USB devices. The > > current serial port handling code is in serialio.c in this > > directory." > > > > Drivers for all external synths now use TTY to communcate with the > > devices. Only ones still using direct communication with hardware > > ports are internal synths: acntpc, decpc, dtlk and keypc. These are > > typically ISA cards and generally hardware which is difficult to > > make work. We can leave these in staging. > > Ok, that's fine. > > > 2. "Some places are currently using in_atomic() because speakup > > functions are called in various contexts, and a couple of things > > can't happen in these cases. Pushing work to some worker thread > > would probably help, as was already done for the serial port > > driving part." > > > > There aren't any uses of in_atomic anymore. Commit d7500135802c > > "Staging: speakup: Move pasting into a work item" was the last one > > that removed such uses. > > Great, let's remove that todo item then. > > > 3. "There is a duplication of the selection functions in > > selections.c. These functions should get exported from > > drivers/char/selection.c (clear_selection notably) and used from > > there instead." > > > > This is yet to be done. I guess drivers/char/selection.c is now > > under drivers/tty/vt/selection.c. > > Yes, someone should update the todo item :) > > > 4. "The kobjects may have to move to a more proper place in /sys.The > > discussion on lkml resulted to putting speech synthesizers in the > > "speech" class, and the speakup screen reader itself > > into /sys/class/vtconsole/vtcon0/speakup, the nasty path being > > handled by userland tools." > > > > Although this makes logical sense, the change will mean changing > > interface with userspace and hence the user space tools. I tried to > > search the lkml discussion but couldn't find it. It will be good to > > know your thoughts on this. > > I don't remember, sorry. I can review the kobject/sysfs usage if you > think it is "good enough" now and see if I find anything > objectionable. > > > Finally there is an issue where text in output buffer sometimes gets > > garbled on SMP systems, but we can continue working on it after the > > driver is moved out of staging, if that's okay. Basically we need a > > reproducer of this issue. > > > > In addition to above, there are likely code style issues which will > > need to be fixed. > > > > We are very keen to get speakup out of staging both, for settling > > the driver but also for getting included in distros which build > > only the mainline drivers. > > That's great, I am glad to see this happen. How about work on the > selection thing and then I can review the kobject stuff in a few > weeks, and then we can start moving things for 5.2? Perfect. I'll start looking into selection refactor now. Thanks very much! Okash
Re: Staging status of speakup
Hi, On Fri, 15 Mar 2019 18:19:39 -0700 Chris Brannon wrote: > Okash Khawaja writes: > > > Finally there is an issue where text in output buffer sometimes gets > > garbled on SMP systems, but we can continue working on it after the > > driver is moved out of staging, if that's okay. Basically we need a > > reproducer of this issue. > > What kind of reproducer do you need here? It's straightforward to > reproduce in casual use, at least with a software synthesizer. I > don't know whether it affects hardware synths. I meant if we can reproduce it at will. Then we will be very close to the root cause of the race which is what it seems like. Btw I haven't started investigating it yet. Thanks, Okash
Staging status of speakup
Hi, We have made progress on the items in TODO file of speakup driver in staging directory and wanted to get some clarity on the remaining items. Below is a summary of status of each item along with the quotes from TODO file. 1. "The first issue has to do with the way speakup communicates with serial ports. Currently, we communicate directly with the hardware ports. This however conflicts with the standard serial port drivers, which poses various problems. This is also not working for modern hardware such as PCI-based serial ports. Also, there is not a way we can communicate with USB devices. The current serial port handling code is in serialio.c in this directory." Drivers for all external synths now use TTY to communcate with the devices. Only ones still using direct communication with hardware ports are internal synths: acntpc, decpc, dtlk and keypc. These are typically ISA cards and generally hardware which is difficult to make work. We can leave these in staging. 2. "Some places are currently using in_atomic() because speakup functions are called in various contexts, and a couple of things can't happen in these cases. Pushing work to some worker thread would probably help, as was already done for the serial port driving part." There aren't any uses of in_atomic anymore. Commit d7500135802c "Staging: speakup: Move pasting into a work item" was the last one that removed such uses. 3. "There is a duplication of the selection functions in selections.c. These functions should get exported from drivers/char/selection.c (clear_selection notably) and used from there instead." This is yet to be done. I guess drivers/char/selection.c is now under drivers/tty/vt/selection.c. 4. "The kobjects may have to move to a more proper place in /sys.The discussion on lkml resulted to putting speech synthesizers in the "speech" class, and the speakup screen reader itself into /sys/class/vtconsole/vtcon0/speakup, the nasty path being handled by userland tools." Although this makes logical sense, the change will mean changing interface with userspace and hence the user space tools. I tried to search the lkml discussion but couldn't find it. It will be good to know your thoughts on this. Finally there is an issue where text in output buffer sometimes gets garbled on SMP systems, but we can continue working on it after the driver is moved out of staging, if that's okay. Basically we need a reproducer of this issue. In addition to above, there are likely code style issues which will need to be fixed. We are very keen to get speakup out of staging both, for settling the driver but also for getting included in distros which build only the mainline drivers. Thank you, Okash
[PATCH bpf 0/1] bpf: btf: Fix endianness of bitfields
Hi,
This patch fixes endianness bug in btf_int_bits_seq_show(). Jakub
Kicinski pointed out in a separate patch review for bpftool ("bpf: btf:
add btf print functionality") that parsing of bitfield might not work
on big endian machine. Similar parsing is performed in
btf_int_bits_seq_show() and this patch fixes it.
Thanks,
Okash
[PATCH bpf 1/1] bpf: btf: Fix bitfield extraction for big endian
When extracting bitfield from a number, btf_int_bits_seq_show() builds
a mask and accesses least significant byte of the number in a way
specific to little-endian. This patch fixes that by checking endianness
of the machine and then shifting left and right the unneeded bits.
Thanks to Martin Lau for the help in navigating potential pitfalls when
dealing with endianess and for the final solution.
Fixes: b00b8daec828 ("bpf: btf: Add pretty print capability for data with BTF
type info")
Signed-off-by: Okash Khawaja
---
kernel/bpf/btf.c | 32 +++-
1 file changed, 15 insertions(+), 17 deletions(-)
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -162,6 +162,8 @@
#define BITS_ROUNDDOWN_BYTES(bits) ((bits) >> 3)
#define BITS_ROUNDUP_BYTES(bits) \
(BITS_ROUNDDOWN_BYTES(bits) + !!BITS_PER_BYTE_MASKED(bits))
+const int one = 1;
+#define is_big_endian() ((*(char *)&one) == 0)
#define BTF_INFO_MASK 0x0f00
#define BTF_INT_MASK 0x0fff
@@ -991,16 +993,13 @@ static void btf_int_bits_seq_show(const
void *data, u8 bits_offset,
struct seq_file *m)
{
+ u8 left_shift_bits, right_shift_bits;
u32 int_data = btf_type_int(t);
u16 nr_bits = BTF_INT_BITS(int_data);
u16 total_bits_offset;
u16 nr_copy_bytes;
u16 nr_copy_bits;
- u8 nr_upper_bits;
- union {
- u64 u64_num;
- u8 u8_nums[8];
- } print_num;
+ u64 print_num;
total_bits_offset = bits_offset + BTF_INT_OFFSET(int_data);
data += BITS_ROUNDDOWN_BYTES(total_bits_offset);
@@ -1008,21 +1007,20 @@ static void btf_int_bits_seq_show(const
nr_copy_bits = nr_bits + bits_offset;
nr_copy_bytes = BITS_ROUNDUP_BYTES(nr_copy_bits);
- print_num.u64_num = 0;
- memcpy(&print_num.u64_num, data, nr_copy_bytes);
-
- /* Ditch the higher order bits */
- nr_upper_bits = BITS_PER_BYTE_MASKED(nr_copy_bits);
- if (nr_upper_bits) {
- /* We need to mask out some bits of the upper byte. */
- u8 mask = (1 << nr_upper_bits) - 1;
-
- print_num.u8_nums[nr_copy_bytes - 1] &= mask;
+ print_num = 0;
+ memcpy(&print_num, data, nr_copy_bytes);
+ if (is_big_endian()) {
+ left_shift_bits = bits_offset;
+ right_shift_bits = BITS_PER_U64 - nr_bits;
+ } else {
+ left_shift_bits = BITS_PER_U64 - nr_copy_bits;
+ right_shift_bits = BITS_PER_U64 - nr_bits;
}
- print_num.u64_num >>= bits_offset;
+ print_num <<= left_shift_bits;
+ print_num >>= right_shift_bits;
- seq_printf(m, "0x%llx", print_num.u64_num);
+ seq_printf(m, "0x%llx", print_num);
}
static void btf_int_seq_show(const struct btf *btf, const struct btf_type *t,
[PATCH bpf-next v3 0/3] bpf: btf: print bpftool map data with btf
Hi, This v3 contains incorporates feedback from v2, including a fix for big endian when extracting bitfields. Below is a summary of all changes. patch 1: - use kernel integer types instead of stdint patch 2: - change stdint types to kernel equivalents - remove variable ret from btf_dumper_modifier() - remove unnecessary parentheses in btf_dumper_enum() - remove unnecessary parentheses in btf_dumper_array() - change integer types from explicitly sized to int in btf_dumper_int_bits - fix btf_dumper_int_bits() so it works for little and big endian - remove double space in btf_dumper_int() - print non-printable characters as string - remove ret variable from btf_dumper_int() - don't initialise variable with function which needs error check in btf_dumper_struct() - use a temp variable to avoid multi-line statement in btf_dumper_struct() - call jsonw_end_object before returning in error case in btf_dumper_struct() - print something in case of BTF_KIND_FWD in btf_dumper_do_type() - return directly from cases in switch-case and save 9 LOC in btf_dumper_do_type() - remove check for null argument in btf_dumper_type() - remove header file btf_dumper.h and move declarations to main.h patch 3: - change stdint types to kernel equivalents - keep header includes in alphabetical order - use goto in do_dump_btf() to ensure jsonw_end_object() in cases of error - don't initialise variable with functions that can fail in get_btf() - remove json-breaking printf in get_btf() - refactor so that there isn't too much code in if (!err) case in do_lookup() Thanks, Okash
[PATCH bpf-next v3 2/3] bpf: btf: add btf print functionality
This consumes functionality exported in the previous patch. It does the
main job of printing with BTF data. This is used in the following patch
to provide a more readable output of a map's dump. It relies on
json_writer to do json printing. Below is sample output where map keys
are ints and values are of type struct A:
typedef int int_type;
enum E {
E0,
E1,
};
struct B {
int x;
int y;
};
struct A {
int m;
unsigned long long n;
char o;
int p[8];
int q[4][8];
enum E r;
void *s;
struct B t;
const int u;
int_type v;
unsigned int w1: 3;
unsigned int w2: 3;
};
$ sudo bpftool map dump id 14
[{
"key": 0,
"value": {
"m": 1,
"n": 2,
"o": "c",
"p": [15,16,17,18,15,16,17,18
],
"q": [[25,26,27,28,25,26,27,28
],[35,36,37,38,35,36,37,38
],[45,46,47,48,45,46,47,48
],[55,56,57,58,55,56,57,58
]
],
"r": 1,
"s": 0x7ffd80531cf8,
"t": {
"x": 5,
"y": 10
},
"u": 100,
"v": 20,
"w1": 0x7,
"w2": 0x3
}
}
]
This patch uses json's {} and [] to imply struct/union and array. More
explicit information can be added later. For example, a command line
option can be introduced to print whether a key or value is struct
or union, name of a struct etc. This will however come at the expense
of duplicating info when, for example, printing an array of structs.
enums are printed as ints without their names.
Signed-off-by: Okash Khawaja
Acked-by: Martin KaFai Lau
---
tools/bpf/bpftool/btf_dumper.c | 253 +
tools/bpf/bpftool/main.h | 15 ++
2 files changed, 268 insertions(+)
--- /dev/null
+++ b/tools/bpf/bpftool/btf_dumper.c
@@ -0,0 +1,253 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2018 Facebook */
+
+#include
+#include
+#include /* for (FILE *) used by json_writer */
+#include
+#include
+#include
+
+#include "btf.h"
+#include "json_writer.h"
+#include "main.h"
+
+#define BITS_PER_BYTE_MASK (BITS_PER_BYTE - 1)
+#define BITS_PER_BYTE_MASKED(bits) ((bits) & BITS_PER_BYTE_MASK)
+#define BITS_ROUNDDOWN_BYTES(bits) ((bits) >> 3)
+#define BITS_ROUNDUP_BYTES(bits) \
+ (BITS_ROUNDDOWN_BYTES(bits) + !!BITS_PER_BYTE_MASKED(bits))
+const int one = 1;
+#define is_big_endian() ((*(char *)&one) == 0)
+
+static int btf_dumper_do_type(const struct btf_dumper *d, __u32 type_id,
+ __u8 bit_offset, const void *data);
+
+static void btf_dumper_ptr(const void *data, json_writer_t *jw,
+ bool is_plain_text)
+{
+ if (is_plain_text)
+ jsonw_printf(jw, "%p", *((unsigned long *)data));
+ else
+ jsonw_printf(jw, "%u", *((unsigned long *)data));
+}
+
+static int btf_dumper_modifier(const struct btf_dumper *d, __u32 type_id,
+ const void *data)
+{
+ int actual_type_id;
+
+ actual_type_id = btf__resolve_type(d->btf, type_id);
+ if (actual_type_id < 0)
+ return actual_type_id;
+
+ return btf_dumper_do_type(d, actual_type_id, 0, data);
+}
+
+static void btf_dumper_enum(const void *data, json_writer_t *jw)
+{
+ jsonw_printf(jw, "%d", *(int *)data);
+}
+
+static int btf_dumper_array(const struct btf_dumper *d, __u32 type_id,
+ const void *data)
+{
+ const struct btf_type *t = btf__type_by_id(d->btf, type_id);
+ struct btf_array *arr = (struct btf_array *)(t + 1);
+ long long elem_size;
+ int ret = 0;
+ __u32 i;
+
+ elem_size = btf__resolve_size(d->btf, arr->type);
+ if (elem_size < 0)
+ return elem_size;
+
+ jsonw_start_array(d->jw);
+ for (i = 0; i < arr->nelems; i++) {
+ ret = btf_dumper_do_type(d, arr->type, 0,
+data + i * elem_size);
+ if (ret)
+ break;
+ }
+
+ jsonw_end_array(d->jw);
+ return ret;
+}
+
+static void btf_dumper_int_bits(__u32 int_type, __u8 bit_offset,
+ const void *data, json_writer_t *jw,
+ bool is_plain_text)
+{
+ int left_shift_bits, right_shift_bits;
+ int nr_bits = BTF_INT_BITS(int_type);
+ int total_bits_offset;
+ int bytes_to_copy;
+ int bits_to_copy;
+ __u64 print_num;
+
+ total_bits_offset
[PATCH bpf-next v3 3/3] bpf: btf: print map dump and lookup with btf info
This patch augments the output of bpftool's map dump and map lookup
commands to print data along side btf info, if the correspondin btf
info is available. The outputs for each of map dump and map lookup
commands are augmented in two ways:
1. when neither of -j and -p are supplied, btf-ful map data is printed
whose aim is human readability. This means no commitments for json- or
backward- compatibility.
2. when either -j or -p are supplied, a new json object named
"formatted" is added for each key-value pair. This object contains the
same data as the key-value pair, but with btf info. "formatted" object
promises json- and backward- compatibility. Below is a sample output.
$ bpftool map dump -p id 8
[{
"key": ["0x0f","0x00","0x00","0x00"
],
"value": ["0x03", "0x00", "0x00", "0x00", ...
],
"formatted": {
"key": 15,
"value": {
"int_field": 3,
...
}
}
}
]
This patch calls btf_dumper introduced in previous patch to accomplish
the above. Indeed, btf-ful info is only displayed if btf data for the
given map is available. Otherwise existing output is displayed as-is.
Signed-off-by: Okash Khawaja
---
tools/bpf/bpftool/map.c | 207
1 file changed, 191 insertions(+), 16 deletions(-)
--- a/tools/bpf/bpftool/map.c
+++ b/tools/bpf/bpftool/map.c
@@ -34,6 +34,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -44,6 +45,8 @@
#include
+#include "btf.h"
+#include "json_writer.h"
#include "main.h"
static const char * const map_type_name[] = {
@@ -148,8 +151,111 @@ int map_parse_fd_and_info(int *argc, cha
return fd;
}
+static int do_dump_btf(const struct btf_dumper *d,
+ struct bpf_map_info *map_info, void *key,
+ void *value)
+{
+ int ret;
+
+ /* start of key-value pair */
+ jsonw_start_object(d->jw);
+
+ jsonw_name(d->jw, "key");
+
+ ret = btf_dumper_type(d, map_info->btf_key_type_id, key);
+ if (ret)
+ goto err_end_obj;
+
+ jsonw_name(d->jw, "value");
+
+ ret = btf_dumper_type(d, map_info->btf_value_type_id, value);
+
+err_end_obj:
+ /* end of key-value pair */
+ jsonw_end_object(d->jw);
+
+ return ret;
+}
+
+static struct btf *get_btf(struct bpf_map_info *map_info)
+{
+ struct bpf_btf_info btf_info = { 0 };
+ __u32 len = sizeof(btf_info);
+ struct btf *btf = NULL;
+ __u32 last_size;
+ int btf_fd;
+ void *ptr;
+ int err;
+
+ btf_fd = bpf_btf_get_fd_by_id(map_info->btf_id);
+ if (btf_fd < 0)
+ return NULL;
+
+ /* we won't know btf_size until we call bpf_obj_get_info_by_fd(). so
+* let's start with a sane default - 4KiB here - and resize it only if
+* bpf_obj_get_info_by_fd() needs a bigger buffer.
+*/
+ btf_info.btf_size = 4096;
+ last_size = btf_info.btf_size;
+ ptr = malloc(last_size);
+ if (!ptr) {
+ p_err("unable to allocate memory for debug info");
+ goto exit_free;
+ }
+
+ bzero(ptr, last_size);
+ btf_info.btf = ptr_to_u64(ptr);
+ err = bpf_obj_get_info_by_fd(btf_fd, &btf_info, &len);
+
+ if (!err && btf_info.btf_size > last_size) {
+ void *temp_ptr;
+
+ last_size = btf_info.btf_size;
+ temp_ptr = realloc(ptr, last_size);
+ if (!temp_ptr) {
+ p_err("unable to re-allocate memory for debug info");
+ goto exit_free;
+ }
+ ptr = temp_ptr;
+ bzero(ptr, last_size);
+ btf_info.btf = ptr_to_u64(ptr);
+ err = bpf_obj_get_info_by_fd(btf_fd, &btf_info, &len);
+ }
+
+ if (err || btf_info.btf_size > last_size) {
+ p_info("can't get btf info. debug info won't be displayed.
error: %s",
+ err ? strerror(errno) : "exceeds size retry");
+ goto exit_free;
+ }
+
+ btf = btf__new((__u8 *)btf_info.btf,
+ btf_info.btf_size, NULL);
+ if (IS_ERR(btf)) {
+ p_info("error when initialising btf: %s\n",
+ strerror(PTR_ERR(btf)));
+ btf = NULL;
+ }
+
+exit_free:
+ close(btf_fd);
+ free(ptr);
+
+ return btf;
+}
+
+static json_writer_t *get_btf_writer(void)
+{
+ json_writer_t *jw = jsonw_new(stdout);
+
+ if (!jw)
+
Re: [PATCH bpf-next 3/3] bpf: btf: json print map dump with btf info
On Thu, Jun 21, 2018 at 12:22:52AM +0100, Song Liu wrote:
>
>
> > On Jun 20, 2018, at 1:30 PM, Okash Khawaja wrote:
> >
> > This patch modifies `bpftool map dump [-j|-p] id ` to json-
> > print and pretty-json-print map dump. It calls btf_dumper introduced in
> > previous patch to accomplish this.
> >
> > The patch only prints debug info when -j or -p flags are supplied. Then
> > too, if the map has associated btf data loaded. Otherwise the usual
> > debug-less output is printed.
> >
> > Signed-off-by: Okash Khawaja
> > Acked-by: Martin KaFai Lau
> >
> > ---
> > tools/bpf/bpftool/map.c | 94
> > ++--
> > 1 file changed, 91 insertions(+), 3 deletions(-)
> >
> > --- a/tools/bpf/bpftool/map.c
> > +++ b/tools/bpf/bpftool/map.c
> > @@ -43,9 +43,13 @@
> > #include
> > #include
> > #include
> > +#include
> >
> > #include
> >
> > +#include "json_writer.h"
> > +#include "btf.h"
> > +#include "btf_dumper.h"
> > #include "main.h"
> >
> > static const char * const map_type_name[] = {
> > @@ -508,6 +512,83 @@ static int do_show(int argc, char **argv
> > return errno == ENOENT ? 0 : -1;
> > }
> >
> > +
> > +static int do_dump_btf(struct btf *btf, struct bpf_map_info *map_info,
> > + void *key, void *value)
> > +{
> > + int ret;
> > +
> > + jsonw_start_object(json_wtr);
> > + jsonw_name(json_wtr, "key");
> > +
> > + ret = btf_dumper_type(btf, json_wtr, map_info->btf_key_type_id, key);
> > + if (ret)
> > + goto out;
> > +
> > + jsonw_end_object(json_wtr);
> > +
> > + jsonw_start_object(json_wtr);
> > + jsonw_name(json_wtr, "value");
> > +
> > + ret = btf_dumper_type(btf, json_wtr, map_info->btf_value_type_id,
> > + value);
> > +
> > +out:
> > + /* end of root object */
> > + jsonw_end_object(json_wtr);
> > +
> > + return ret;
> > +}
>
> Please add some tests for do_dump_btf(), including some invalid data
> type/kind.
Sure. I was wondering if we can follow this up with tests afterwards.
However if they are essential now, I can add them.
>
> > +
> > +static struct btf *get_btf(struct bpf_map_info *map_info)
> > +{
> > + int btf_fd = bpf_btf_get_fd_by_id(map_info->btf_id);
> > + struct bpf_btf_info btf_info = { 0 };
> > + __u32 len = sizeof(btf_info);
> > + uint32_t last_size;
> > + int err;
> > + struct btf *btf = NULL;
> > + void *ptr = NULL, *temp_ptr;
> > +
> > + if (btf_fd < 0)
> > + return NULL;
> > +
> > + btf_info.btf_size = 4096;
>
> Is btf_size always a constant of 4096?
We start out with 4096 and in the loop below we increase the size if
needed. It seems like a sane valeu to start with. However if there are
better ideas for start value, we can discuss them.
>
> > + do {
> > + last_size = btf_info.btf_size;
> > + temp_ptr = realloc(ptr, last_size);
> > + if (!temp_ptr) {
> > + p_err("unable allocate memory for debug info.");
> > + goto exit_free;
> > + }
> > +
> > + ptr = temp_ptr;
> > + bzero(ptr, last_size);
> > + btf_info.btf = ptr_to_u64(ptr);
> > + err = bpf_obj_get_info_by_fd(btf_fd, &btf_info, &len);
> > + } while (!err && btf_info.btf_size > last_size && last_size == 4096);
> > +
> > + if (err || btf_info.btf_size > last_size) {
> > + p_info("can't get btf info. debug info won't be displayed.
> > error: %s",
> > + err ? strerror(errno) : "exceeds size retry");
> > + goto exit_free;
> > + }
> > +
> > + btf = btf__new((uint8_t *) btf_info.btf,
> > + btf_info.btf_size, NULL);
> > + if (IS_ERR(btf)) {
> > + printf("error when initialising btf: %s\n",
> > + strerror(PTR_ERR(btf)));
> > + btf = NULL;
> > + }
> > +
> > +exit_free:
> > + close(btf_fd);
> > + free(ptr);
> > +
> > + return btf;
> > +}
> > +
> > static int do_dump(int argc, char **argv)
> > {
> > void *key, *value, *prev_key;
> > @@
[PATCH bpf-next 3/3] bpf: btf: json print map dump with btf info
This patch modifies `bpftool map dump [-j|-p] id ` to json-
print and pretty-json-print map dump. It calls btf_dumper introduced in
previous patch to accomplish this.
The patch only prints debug info when -j or -p flags are supplied. Then
too, if the map has associated btf data loaded. Otherwise the usual
debug-less output is printed.
Signed-off-by: Okash Khawaja
Acked-by: Martin KaFai Lau
---
tools/bpf/bpftool/map.c | 94 ++--
1 file changed, 91 insertions(+), 3 deletions(-)
--- a/tools/bpf/bpftool/map.c
+++ b/tools/bpf/bpftool/map.c
@@ -43,9 +43,13 @@
#include
#include
#include
+#include
#include
+#include "json_writer.h"
+#include "btf.h"
+#include "btf_dumper.h"
#include "main.h"
static const char * const map_type_name[] = {
@@ -508,6 +512,83 @@ static int do_show(int argc, char **argv
return errno == ENOENT ? 0 : -1;
}
+
+static int do_dump_btf(struct btf *btf, struct bpf_map_info *map_info,
+ void *key, void *value)
+{
+ int ret;
+
+ jsonw_start_object(json_wtr);
+ jsonw_name(json_wtr, "key");
+
+ ret = btf_dumper_type(btf, json_wtr, map_info->btf_key_type_id, key);
+ if (ret)
+ goto out;
+
+ jsonw_end_object(json_wtr);
+
+ jsonw_start_object(json_wtr);
+ jsonw_name(json_wtr, "value");
+
+ ret = btf_dumper_type(btf, json_wtr, map_info->btf_value_type_id,
+ value);
+
+out:
+ /* end of root object */
+ jsonw_end_object(json_wtr);
+
+ return ret;
+}
+
+static struct btf *get_btf(struct bpf_map_info *map_info)
+{
+ int btf_fd = bpf_btf_get_fd_by_id(map_info->btf_id);
+ struct bpf_btf_info btf_info = { 0 };
+ __u32 len = sizeof(btf_info);
+ uint32_t last_size;
+ int err;
+ struct btf *btf = NULL;
+ void *ptr = NULL, *temp_ptr;
+
+ if (btf_fd < 0)
+ return NULL;
+
+ btf_info.btf_size = 4096;
+ do {
+ last_size = btf_info.btf_size;
+ temp_ptr = realloc(ptr, last_size);
+ if (!temp_ptr) {
+ p_err("unable allocate memory for debug info.");
+ goto exit_free;
+ }
+
+ ptr = temp_ptr;
+ bzero(ptr, last_size);
+ btf_info.btf = ptr_to_u64(ptr);
+ err = bpf_obj_get_info_by_fd(btf_fd, &btf_info, &len);
+ } while (!err && btf_info.btf_size > last_size && last_size == 4096);
+
+ if (err || btf_info.btf_size > last_size) {
+ p_info("can't get btf info. debug info won't be displayed.
error: %s",
+ err ? strerror(errno) : "exceeds size retry");
+ goto exit_free;
+ }
+
+ btf = btf__new((uint8_t *) btf_info.btf,
+ btf_info.btf_size, NULL);
+ if (IS_ERR(btf)) {
+ printf("error when initialising btf: %s\n",
+ strerror(PTR_ERR(btf)));
+ btf = NULL;
+ }
+
+exit_free:
+ close(btf_fd);
+ free(ptr);
+
+ return btf;
+}
+
static int do_dump(int argc, char **argv)
{
void *key, *value, *prev_key;
@@ -516,6 +597,7 @@ static int do_dump(int argc, char **argv
__u32 len = sizeof(info);
int err;
int fd;
+ struct btf *btf = NULL;
if (argc != 2)
usage();
@@ -538,6 +620,8 @@ static int do_dump(int argc, char **argv
goto exit_free;
}
+ btf = get_btf(&info);
+
prev_key = NULL;
if (json_output)
jsonw_start_array(json_wtr);
@@ -550,9 +634,12 @@ static int do_dump(int argc, char **argv
}
if (!bpf_map_lookup_elem(fd, key, value)) {
- if (json_output)
- print_entry_json(&info, key, value);
- else
+ if (json_output) {
+ if (btf)
+ do_dump_btf(btf, &info, key, value);
+ else
+ print_entry_json(&info, key, value);
+ } else
print_entry_plain(&info, key, value);
} else {
if (json_output) {
@@ -584,6 +671,7 @@ exit_free:
free(key);
free(value);
close(fd);
+ btf__free(btf);
return err;
}
[patch 0/1] staging: speakup: fix speakup-r empty line lockup
Hi,
This patch fixes kernel lockup shown by lockdep log below. Further
details are in patch header.
Samuel, please note that I removed initialisation of the static int
in_keyboard_notifier to zero as that is not required and reported as
error by checkpatch.
Thanks,
Okash
[ 1293.803242]
[ 1293.803304] WARNING: possible recursive locking detected
[ 1293.803369] 4.12.2-ARCH+ #7 Tainted: G C O
[ 1293.803442]
[ 1293.803504] swapper/0/0 is trying to acquire lock:
[ 1293.803562] (&(&dev->event_lock)->rlock){-.-...}, at: []
input_event+0x3e/0x80
[ 1293.803671]
[ 1293.803671] but task is already holding lock:
[ 1293.803740] (&(&dev->event_lock)->rlock){-.-...}, at: []
input_event+0x3e/0x80
[ 1293.803843]
[ 1293.803843] other info that might help us debug this:
[ 1293.803916] Possible unsafe locking scenario:
[ 1293.803916]
[ 1293.803984]CPU0
[ 1293.804021]
[ 1293.804054] lock(&(&dev->event_lock)->rlock);
[ 1293.804109] lock(&(&dev->event_lock)->rlock);
[ 1293.804164]
[ 1293.804164] *** DEADLOCK ***
[ 1293.804164]
[ 1293.804233] May be due to missing lock nesting notation
[ 1293.804233]
[ 1293.804311] 6 locks held by swapper/0/0:
[ 1293.804359] #0: (&serio->lock){-.-...}, at: []
serio_interrupt+0x2a/0x90 [serio]
[ 1293.804469] #1: (&(&dev->event_lock)->rlock){-.-...}, at:
[] input_event+0x3e/0x80
[ 1293.804577] #2: (rcu_read_lock){..}, at: []
input_pass_values.part.1+0x5/0x260
[ 1293.804685] #3: (kbd_event_lock){-.-...}, at: []
kbd_event+0x3c/0x710
[ 1293.804781] #4: (rcu_read_lock){..}, at: []
__atomic_notifier_call_chain+0x5/0x110
[ 1293.804898] #5: (speakup_info.spinlock){-.-...}, at: []
keyboard_notifier_call+0xfc/0xcc0 [speakup]
[ 1293.805028]
[ 1293.805028] stack backtrace:
[ 1293.805084] CPU: 0 PID: 0 Comm: swapper/0 Tainted: G C O
4.12.2-ARCH+ #7
[ 1293.805168] Hardware name: Dell Inc. Inspiron 7559/0H0CC0, BIOS 1.2.2
01/22/2017
[ 1293.805250] Call Trace:
[ 1293.805284]
[ 1293.805320] dump_stack+0x8e/0xcd
[ 1293.805366] __lock_acquire+0x87d/0x1a20
[ 1293.805423] ? native_apic_wait_icr_idle+0x1f/0x30
[ 1293.805482] ? arch_irq_work_raise+0x34/0x40
[ 1293.805537] lock_acquire+0xa5/0x250
[ 1293.805583] ? lock_acquire+0xa5/0x250
[ 1293.805632] ? input_event+0x3e/0x80
[ 1293.805683] _raw_spin_lock_irqsave+0x4d/0x90
[ 1293.805736] ? input_event+0x3e/0x80
[ 1293.805782] input_event+0x3e/0x80
[ 1293.805833] speakup_fake_down_arrow2+0x57/0xd0 [speakup]
[ 1293.805902] read_all_doc+0xd7/0xe0 [speakup]
[ 1293.805960] ? kbd_fakekey+0x40/0x40 [speakup]
[ 1293.806020] keyboard_notifier_call+0xbe8/0xcc0 [speakup]
[ 1293.806085] ? lock_acquire+0xa5/0x250
[ 1293.806136] notifier_call_chain+0x4a/0x70
[ 1293.806191] __atomic_notifier_call_chain+0x72/0x110
[ 1293.806254] atomic_notifier_call_chain+0x16/0x20
[ 1293.806311] kbd_event+0x320/0x710
[ 1293.806361] input_to_handler+0xdb/0xf0
[ 1293.806411] input_pass_values.part.1+0x1b3/0x260
[ 1293.806469] input_handle_event+0xe4/0x150
[ 1293.806608] input_event+0x52/0x80
[ 1293.806660] atkbd_interrupt+0x50a/0x7c0 [atkbd]
[ 1293.806719] serio_interrupt+0x46/0x90 [serio]
[ 1293.806778] i8042_interrupt+0x20d/0x3b0 [i8042]
[ 1293.806839] __handle_irq_event_percpu+0x45/0x390
[ 1293.806899] handle_irq_event_percpu+0x32/0x80
[ 1293.806955] handle_irq_event+0x39/0x60
[ 1293.807007] handle_edge_irq+0x78/0x130
[ 1293.808787] handle_irq+0x1a/0x30
[ 1293.809755] do_IRQ+0x58/0x110
[ 1293.810712] common_interrupt+0x93/0x93
[ 1293.811644] RIP: 0010:cpuidle_enter_state+0x143/0x390
[ 1293.812575] RSP: 0018:81c03dc0 EFLAGS: 0202 ORIG_RAX:
ffce
[ 1293.813518] RAX: 81c18500 RBX: 012d3ca4a19b RCX:
[ 1293.814462] RDX: 81c18500 RSI: 0001 RDI: 81c18500
[ 1293.815398] RBP: 81c03e00 R08: cccd R09:
[ 1293.816335] R10: R11: R12: 8804707e3100
[ 1293.817261] R13: R14: 0008 R15: 81cc7898
[ 1293.818179]
[ 1293.819081] cpuidle_enter+0x17/0x20
[ 1293.819986] call_cpuidle+0x23/0x40
[ 1293.820884] do_idle+0x18f/0x1f0
[ 1293.821776] cpu_startup_entry+0x71/0x80
[ 1293.822662] rest_init+0x131/0x140
[ 1293.823547] start_kernel+0x44d/0x46e
[ 1293.824426] ? early_idt_handler_array+0x120/0x120
[ 1293.825326] x86_64_start_reservations+0x2f/0x31
[ 1293.826186] x86_64_start_kernel+0x141/0x164
[ 1293.827045] secondary_startup_64+0x9f/0x9f
[patch 1/1] [patch] staging: speakup: fix speakup-r empty line lockup
When cursor is at beginning of an empty or whitespace-only line and
speakup-r typed, kernel locks up. This happens because deadlock of in
input_event function over dev->event_lock, as demonstrated by lockdep
logs. The reason for that is speakup simulates a down arrow - because
cursor is at an empty line - while inside key press notifier handler
which is ultimately triggered from input_event function. The simulated
key press leads to input_event being called again, this time under its
own context. So the spinlock is dev->event_lock is acquired while still
being held.
This patch ensures that key press is not simulated from inside key press
notifier handler. Instead it delegates to cursor_timer. It starts the
timer and passes RA_DOWN_ARROW as argument. When timer handler runs and
sees RA_DOWN_ARROW, it will then call kbd_fakekey2(RA_DOWN_ARROW) which
will correctly simulate the keypress inside timer context.
When not inside key press notifier callback, the behaviour will remain
the same as before this patch.
Signed-off-by: Okash Khawaja
Reviewed-by: Samuel Thibault
---
drivers/staging/speakup/main.c | 15 ---
1 file changed, 12 insertions(+), 3 deletions(-)
--- a/drivers/staging/speakup/main.c
+++ b/drivers/staging/speakup/main.c
@@ -1376,6 +1376,8 @@ static void reset_highlight_buffers(stru
static int read_all_key;
+static int in_keyboard_notifier;
+
static void start_read_all_timer(struct vc_data *vc, int command);
enum {
@@ -1408,7 +1410,10 @@ static void read_all_doc(struct vc_data
cursor_track = read_all_mode;
spk_reset_index_count(0);
if (get_sentence_buf(vc, 0) == -1) {
- kbd_fakekey2(vc, RA_DOWN_ARROW);
+ del_timer(&cursor_timer);
+ if (!in_keyboard_notifier)
+ speakup_fake_down_arrow();
+ start_read_all_timer(vc, RA_DOWN_ARROW);
} else {
say_sentence_num(0, 0);
synth_insert_next_index(0);
@@ -2212,8 +2217,10 @@ static int keyboard_notifier_call(struct
int ret = NOTIFY_OK;
static int keycode; /* to hold the current keycode */
+ in_keyboard_notifier = 1;
+
if (vc->vc_mode == KD_GRAPHICS)
- return ret;
+ goto out;
/*
* First, determine whether we are handling a fake keypress on
@@ -2225,7 +2232,7 @@ static int keyboard_notifier_call(struct
*/
if (speakup_fake_key_pressed())
- return ret;
+ goto out;
switch (code) {
case KBD_KEYCODE:
@@ -2266,6 +2273,8 @@ static int keyboard_notifier_call(struct
break;
}
}
+out:
+ in_keyboard_notifier = 0;
return ret;
}
[patch] staging: speakup: fix async usb removal
When an external USB synth is unplugged while the module is loaded, we
get a null pointer deref. This is because the tty disappears while
speakup tries to use to to communicate with the synth. This patch fixes
it by checking tty for null before using it. Since tty can become null
between the check and its usage, a mutex is introduced. tty usage is
now surrounded by the mutex, as is the code in speakup_ldisc_close which
sets the tty to null. The mutex also serialises calls to tty from
speakup code.
In case of tty being null, this sets synth->alive to zero and restarts
ttys in case they were stopped by speakup.
Signed-off-by: Okash Khawaja
Reviewed-by: Samuel Thibault
---
drivers/staging/speakup/spk_ttyio.c | 50
1 file changed, 50 insertions(+)
--- a/drivers/staging/speakup/spk_ttyio.c
+++ b/drivers/staging/speakup/spk_ttyio.c
@@ -15,6 +15,11 @@ struct spk_ldisc_data {
static struct spk_synth *spk_ttyio_synth;
static struct tty_struct *speakup_tty;
+/* mutex to protect against speakup_tty disappearing from underneath us while
+ * we are using it. this can happen when the device physically unplugged,
+ * while in use. it also serialises access to speakup_tty.
+ */
+static DEFINE_MUTEX(speakup_tty_mutex);
static int ser_to_dev(int ser, dev_t *dev_no)
{
@@ -60,8 +65,10 @@ static int spk_ttyio_ldisc_open(struct t
static void spk_ttyio_ldisc_close(struct tty_struct *tty)
{
+ mutex_lock(&speakup_tty_mutex);
kfree(speakup_tty->disc_data);
speakup_tty = NULL;
+ mutex_unlock(&speakup_tty_mutex);
}
static int spk_ttyio_receive_buf2(struct tty_struct *tty,
@@ -189,9 +196,11 @@ void spk_ttyio_unregister_ldisc(void)
static int spk_ttyio_out(struct spk_synth *in_synth, const char ch)
{
+ mutex_lock(&speakup_tty_mutex);
if (in_synth->alive && speakup_tty && speakup_tty->ops->write) {
int ret = speakup_tty->ops->write(speakup_tty, &ch, 1);
+ mutex_unlock(&speakup_tty_mutex);
if (ret == 0)
/* No room */
return 0;
@@ -207,17 +216,50 @@ static int spk_ttyio_out(struct spk_synt
}
return 1;
}
+
+ mutex_unlock(&speakup_tty_mutex);
+ return 0;
+}
+
+static int check_tty(struct tty_struct *tty)
+{
+ if (!tty) {
+ pr_warn("%s: I/O error, deactivating speakup\n",
+ spk_ttyio_synth->long_name);
+ /* No synth any more, so nobody will restart TTYs, and we thus
+* need to do it ourselves. Now that there is no synth we can
+* let application flood anyway
+*/
+ spk_ttyio_synth->alive = 0;
+ speakup_start_ttys();
+ return 1;
+ }
+
return 0;
}
static void spk_ttyio_send_xchar(char ch)
{
+ mutex_lock(&speakup_tty_mutex);
+ if (check_tty(speakup_tty)) {
+ mutex_unlock(&speakup_tty_mutex);
+ return;
+ }
+
speakup_tty->ops->send_xchar(speakup_tty, ch);
+ mutex_unlock(&speakup_tty_mutex);
}
static void spk_ttyio_tiocmset(unsigned int set, unsigned int clear)
{
+ mutex_lock(&speakup_tty_mutex);
+ if (check_tty(speakup_tty)) {
+ mutex_unlock(&speakup_tty_mutex);
+ return;
+ }
+
speakup_tty->ops->tiocmset(speakup_tty, set, clear);
+ mutex_unlock(&speakup_tty_mutex);
}
static unsigned char ttyio_in(int timeout)
@@ -257,8 +299,16 @@ static unsigned char spk_ttyio_in_nowait
static void spk_ttyio_flush_buffer(void)
{
+ mutex_lock(&speakup_tty_mutex);
+ if (check_tty(speakup_tty)) {
+ mutex_unlock(&speakup_tty_mutex);
+ return;
+ }
+
if (speakup_tty->ops->flush_buffer)
speakup_tty->ops->flush_buffer(speakup_tty);
+
+ mutex_unlock(&speakup_tty_mutex);
}
int spk_ttyio_synth_probe(struct spk_synth *synth)
Re: [patch] staging: speakup: remove support for lp*
Testing has shown that lp* devices don't work correctly with speakup
just yet. That will require some additional work. Until then, this patch
removes code related to that.
Signed-off-by: Okash Khawaja
Reviewed-by: Samuel Thibault
---
drivers/staging/speakup/spk_ttyio.c | 23 +--
1 file changed, 1 insertion(+), 22 deletions(-)
--- a/drivers/staging/speakup/spk_ttyio.c
+++ b/drivers/staging/speakup/spk_ttyio.c
@@ -7,11 +7,6 @@
#include "spk_types.h"
#include "spk_priv.h"
-#define DEV_PREFIX_LP "lp"
-
-static const char * const lp_supported[] = { "acntsa", "bns", "dummy",
- "txprt" };
-
struct spk_ldisc_data {
char buf;
struct semaphore sem;
@@ -36,24 +31,8 @@ static int get_dev_to_use(struct spk_syn
{
/* use ser only when dev is not specified */
if (strcmp(synth->dev_name, SYNTH_DEFAULT_DEV) ||
- synth->ser == SYNTH_DEFAULT_SER) {
- /* for /dev/lp* check if synth is supported */
- if (strncmp(synth->dev_name, DEV_PREFIX_LP,
- strlen(DEV_PREFIX_LP)) == 0)
- if (match_string(lp_supported, ARRAY_SIZE(lp_supported),
- synth->name) < 0) {
- int i;
-
- pr_err("speakup: lp* is only supported on:");
- for (i = 0; i < ARRAY_SIZE(lp_supported); i++)
- pr_cont(" %s", lp_supported[i]);
- pr_cont("\n");
-
- return -ENOTSUPP;
- }
-
+ synth->ser == SYNTH_DEFAULT_SER)
return tty_dev_name_to_number(synth->dev_name, dev_no);
- }
return ser_to_dev(synth->ser, dev_no);
}
[patch] staging: speakup: remove support for lp*
Testing has shown that lp* devices don't work correctly with speakup
just yet. That will require some additional work. Until then, this patch
removes code related to that.
Signed-off-by: Okash Khawaja
---
drivers/staging/speakup/spk_ttyio.c | 23 +--
1 file changed, 1 insertion(+), 22 deletions(-)
--- a/drivers/staging/speakup/spk_ttyio.c
+++ b/drivers/staging/speakup/spk_ttyio.c
@@ -7,11 +7,6 @@
#include "spk_types.h"
#include "spk_priv.h"
-#define DEV_PREFIX_LP "lp"
-
-static const char * const lp_supported[] = { "acntsa", "bns", "dummy",
- "txprt" };
-
struct spk_ldisc_data {
char buf;
struct semaphore sem;
@@ -36,24 +31,8 @@ static int get_dev_to_use(struct spk_syn
{
/* use ser only when dev is not specified */
if (strcmp(synth->dev_name, SYNTH_DEFAULT_DEV) ||
- synth->ser == SYNTH_DEFAULT_SER) {
- /* for /dev/lp* check if synth is supported */
- if (strncmp(synth->dev_name, DEV_PREFIX_LP,
- strlen(DEV_PREFIX_LP)) == 0)
- if (match_string(lp_supported, ARRAY_SIZE(lp_supported),
- synth->name) < 0) {
- int i;
-
- pr_err("speakup: lp* is only supported on:");
- for (i = 0; i < ARRAY_SIZE(lp_supported); i++)
- pr_cont(" %s", lp_supported[i]);
- pr_cont("\n");
-
- return -ENOTSUPP;
- }
-
+ synth->ser == SYNTH_DEFAULT_SER)
return tty_dev_name_to_number(synth->dev_name, dev_no);
- }
return ser_to_dev(synth->ser, dev_no);
}
[patch v3 2/3] staging: speakup: use tty_kopen and tty_kclose
This patch replaces call to tty_open_by_driver with a tty_kopen and uses tty_kclose instead of tty_release_struct to close it. Signed-off-by: Okash Khawaja --- drivers/staging/speakup/spk_ttyio.c |4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) --- a/drivers/staging/speakup/spk_ttyio.c +++ b/drivers/staging/speakup/spk_ttyio.c @@ -158,7 +158,7 @@ static int spk_ttyio_initialise_ldisc(st if (ret) return ret; - tty = tty_open_by_driver(dev, NULL, NULL); + tty = tty_kopen(dev); if (IS_ERR(tty)) return PTR_ERR(tty); @@ -308,7 +308,7 @@ void spk_ttyio_release(void) tty_ldisc_flush(speakup_tty); tty_unlock(speakup_tty); - tty_release_struct(speakup_tty, speakup_tty->index); + tty_kclose(speakup_tty); } EXPORT_SYMBOL_GPL(spk_ttyio_release);
[patch v3 0/3] tty contention resulting from tty_open_by_driver export
Hi, I have updated the patches so that the exclusivity flag is in tty_port. When closing the struct - by calling tty_release_struct - we also need to reset the flag. One way to do that is to reset the flag inside tty_release_struct function, regardless of whether the tty was opened through tty_kopen or not. In order to keep the code clean, I have instead created a separate function called tty_kclose which is the same as tty_release_struct except that it also resets the exclusivity flag. As a result, any changes to tty_release_struct won't be in tty_kclose untless manually added. Please advise on this. Here is a summary of changes when compared to v2: - Patch 1 uses TTY_PORT_KOPENED flag instead of TTY_KOPENED and as a result adds helper functions to read and change the flag - Patch 1 adds tty_kclose function to close tty opened by tty_kopen - Patch 2 calls tty_kclose instead of tty_release_struct Thanks, Okash
[patch v3 3/3] tty: undo export of tty_open_by_driver
Since we have tty_kopen, we no longer need to export tty_open_by_driver.
This patch makes this function static.
Signed-off-by: Okash Khawaja
---
drivers/tty/tty_io.c |3 +--
include/linux/tty.h |5 -
2 files changed, 1 insertion(+), 7 deletions(-)
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -1885,7 +1885,7 @@ EXPORT_SYMBOL_GPL(tty_kopen);
* - concurrent tty driver removal w/ lookup
* - concurrent tty removal from driver table
*/
-struct tty_struct *tty_open_by_driver(dev_t device, struct inode *inode,
+static struct tty_struct *tty_open_by_driver(dev_t device, struct inode *inode,
struct file *filp)
{
struct tty_struct *tty;
@@ -1936,7 +1936,6 @@ out:
tty_driver_kref_put(driver);
return tty;
}
-EXPORT_SYMBOL_GPL(tty_open_by_driver);
/**
* tty_open- open a tty device
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -400,8 +400,6 @@ extern struct tty_struct *get_current_tt
/* tty_io.c */
extern int __init tty_init(void);
extern const char *tty_name(const struct tty_struct *tty);
-extern struct tty_struct *tty_open_by_driver(dev_t device, struct inode *inode,
- struct file *filp);
extern struct tty_struct *tty_kopen(dev_t device);
extern void tty_kclose(struct tty_struct *tty);
extern int tty_dev_name_to_number(const char *name, dev_t *number);
@@ -425,9 +423,6 @@ static inline int __init tty_init(void)
{ return 0; }
static inline const char *tty_name(const struct tty_struct *tty)
{ return "(none)"; }
-static inline struct tty_struct *tty_open_by_driver(dev_t device,
- struct inode *inode, struct file *filp)
-{ return NULL; }
static inline struct tty_struct *tty_kopen(dev_t device)
{ return ERR_PTR(-ENODEV); }
static inline void tty_kclose(struct tty_struct *tty)
[patch v3 1/3] tty: resolve tty contention between kernel and user space
The commit 12e84c71b7d4 ("tty: export tty_open_by_driver") exports
tty_open_by_device to allow tty to be opened from inside kernel which
works fine except that it doesn't handle contention with user space or
another kernel-space open of the same tty. For example, opening a tty
from user space while it is kernel opened results in failure and a
kernel log message about mismatch between tty->count and tty's file
open count.
This patch makes kernel access to tty exclusive, so that if a user
process or kernel opens a kernel opened tty, it gets -EBUSY. It does
this by adding TTY_KOPENED flag to tty->flags. When this flag is set,
tty_open_by_driver returns -EBUSY. Instead of overloading
tty_open_by_driver for both kernel and user space, this
patch creates a separate function tty_kopen which closely follows
tty_open_by_driver. tty_kclose closes the tty opened by tty_kopen.
To address the mismatch between tty->count and #fd's, this patch adds
#kopen's to the count before comparing it with tty->count. That way
check_tty_count reflects correct usage count.
Returning -EBUSY on tty open is a change in the interface. I have
tested this with minicom, picocom and commands like "echo foo >
/dev/ttyS0". They all correctly report "Device or resource busy" when
the tty is already kernel opened.
Signed-off-by: Okash Khawaja
---
drivers/tty/tty_io.c | 100 ---
include/linux/tty.h | 21 ++
2 files changed, 116 insertions(+), 5 deletions(-)
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -280,7 +280,7 @@ static int check_tty_count(struct tty_st
{
#ifdef CHECK_TTY_COUNT
struct list_head *p;
- int count = 0;
+ int count = 0, kopen_count = 0;
spin_lock(&tty->files_lock);
list_for_each(p, &tty->tty_files) {
@@ -291,10 +291,12 @@ static int check_tty_count(struct tty_st
tty->driver->subtype == PTY_TYPE_SLAVE &&
tty->link && tty->link->count)
count++;
- if (tty->count != count) {
- tty_warn(tty, "%s: tty->count(%d) != #fd's(%d)\n",
-routine, tty->count, count);
- return count;
+ if (tty_port_kopened(tty->port))
+ kopen_count++;
+ if (tty->count != (count + kopen_count)) {
+ tty_warn(tty, "%s: tty->count(%d) != (#fd's(%d) +
#kopen's(%d))\n",
+routine, tty->count, count, kopen_count);
+ return (count + kopen_count);
}
#endif
return 0;
@@ -1513,6 +1515,38 @@ static int tty_release_checks(struct tty
}
/**
+ * tty_kclose - closes tty opened by tty_kopen
+ * @tty: tty device
+ *
+ * Performs the final steps to release and free a tty device. It is the
+ * same as tty_release_struct except that it also resets TTY_PORT_KOPENED
+ * flag on tty->port.
+ */
+void tty_kclose(struct tty_struct *tty)
+{
+ /*
+* Ask the line discipline code to release its structures
+*/
+ tty_ldisc_release(tty);
+
+ /* Wait for pending work before tty destruction commmences */
+ tty_flush_works(tty);
+
+ tty_debug_hangup(tty, "freeing structure\n");
+ /*
+* The release_tty function takes care of the details of clearing
+* the slots and preserving the termios structure. The tty_unlock_pair
+* should be safe as we keep a kref while the tty is locked (so the
+* unlock never unlocks a freed tty).
+*/
+ mutex_lock(&tty_mutex);
+ tty_port_set_kopened(tty->port, 0);
+ release_tty(tty, tty->index);
+ mutex_unlock(&tty_mutex);
+}
+EXPORT_SYMBOL_GPL(tty_kclose);
+
+/**
* tty_release_struct - release a tty struct
* @tty: tty device
* @idx: index of the tty
@@ -1786,6 +1820,56 @@ static struct tty_driver *tty_lookup_dri
}
/**
+ * tty_kopen - open a tty device for kernel
+ * @device: dev_t of device to open
+ *
+ * Opens tty exclusively for kernel. Performs the driver lookup,
+ * makes sure it's not already opened and performs the first-time
+ * tty initialization.
+ *
+ * Returns the locked initialized &tty_struct
+ *
+ * Claims the global tty_mutex to serialize:
+ * - concurrent first-time tty initialization
+ * - concurrent tty driver removal w/ lookup
+ * - concurrent tty removal from driver table
+ */
+struct tty_struct *tty_kopen(dev_t device)
+{
+ struct tty_struct *tty;
+ struct tty_driver *driver = NULL;
+ int index = -1;
+
+ mutex_lock(&tty_mutex);
+ driver = tty_lookup_driver(device, NULL, &index);
+ if (IS_ERR(driver)) {
+ mutex_unlock(&tty_mutex);
+
Re: [patch 0/3] Re: tty contention resulting from tty_open_by_device export
On Tue, Jul 18, 2017 at 03:26:37PM +0300, Dan Carpenter wrote:
> > + if (tty) {
> > + /* drop kref from tty_driver_lookup_tty() */
> > + tty_kref_put(tty);
> > + tty = ERR_PTR(-EBUSY);
> > + } else { /* tty_init_dev returns tty with the tty_lock held */
> > + tty = tty_init_dev(driver, index);
> > + tty_port_set_kopened(tty->port, 1);
>^
>
> tty_init_dev() can fail leading to an error pointer dereference here.
Thanks very much. I will check for error pointer here.
Okash
Re: [patch 0/3] Re: tty contention resulting from tty_open_by_device export
On Mon, Jul 17, 2017 at 11:04:38PM +0100, Alan Cox wrote:
>
> > Sure. I can fix the tty->count mismatch based on Alan's suggestion. However
> > I don't understand why the exclusivity flag should belong to tty_port and
> > not tty_struct. It will be good to know why.
>
> We are trying to move all the flags that we can and structs into the
> tty_port, except any that are used internally within the struct tty
> level code. The main reason for this is to make the object lifetimes and
> locking simpler - because the tty_port lasts for the time the hardware is
> present.
I see. That does make sense. I have appended a version of the patch which
adds the flag to tty_port and uses that inside tty_kopen.
>From readability point of view however, I think adding the flag to
tty_struct looks more intuitive. At least for now - until we move
other things from tty_struct to tty_port.
The patch is untested but I can work on this if that fits in with the
plans for tty.
Thanks,
Okash
---
drivers/tty/tty_io.c | 67 +++
include/linux/tty.h | 17
2 files changed, 79 insertions(+), 5 deletions(-)
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -280,7 +280,7 @@ static int check_tty_count(struct tty_st
{
#ifdef CHECK_TTY_COUNT
struct list_head *p;
- int count = 0;
+ int count = 0, kopen_count = 0;
spin_lock(&tty->files_lock);
list_for_each(p, &tty->tty_files) {
@@ -291,10 +291,12 @@ static int check_tty_count(struct tty_st
tty->driver->subtype == PTY_TYPE_SLAVE &&
tty->link && tty->link->count)
count++;
- if (tty->count != count) {
- tty_warn(tty, "%s: tty->count(%d) != #fd's(%d)\n",
-routine, tty->count, count);
- return count;
+ if (tty_port_kopened(tty->port))
+ kopen_count++;
+ if (tty->count != (count + kopen_count)) {
+ tty_warn(tty, "%s: tty->count(%d) != (#fd's(%d) +
#kopen's(%d))\n",
+routine, tty->count, count, kopen_count);
+ return (count + kopen_count);
}
#endif
return 0;
@@ -1522,6 +1524,7 @@ static int tty_release_checks(struct tty
*/
void tty_release_struct(struct tty_struct *tty, int idx)
{
+ // TODO: reset TTY_PORT_KOPENED on tty->port
/*
* Ask the line discipline code to release its structures
*/
@@ -1786,6 +1789,54 @@ static struct tty_driver *tty_lookup_dri
}
/**
+ * tty_kopen - open a tty device for kernel
+ * @device: dev_t of device to open
+ *
+ * Opens tty exclusively for kernel. Performs the driver lookup,
+ * makes sure it's not already opened and performs the first-time
+ * tty initialization.
+ *
+ * Returns the locked initialized &tty_struct
+ *
+ * Claims the global tty_mutex to serialize:
+ * - concurrent first-time tty initialization
+ * - concurrent tty driver removal w/ lookup
+ * - concurrent tty removal from driver table
+ */
+struct tty_struct *tty_kopen(dev_t device)
+{
+ struct tty_struct *tty;
+ struct tty_driver *driver = NULL;
+ int index = -1;
+
+ mutex_lock(&tty_mutex);
+ driver = tty_lookup_driver(device, NULL, &index);
+ if (IS_ERR(driver)) {
+ mutex_unlock(&tty_mutex);
+ return ERR_CAST(driver);
+ }
+
+ /* check whether we're reopening an existing tty */
+ tty = tty_driver_lookup_tty(driver, NULL, index);
+ if (IS_ERR(tty))
+ goto out;
+
+ if (tty) {
+ /* drop kref from tty_driver_lookup_tty() */
+ tty_kref_put(tty);
+ tty = ERR_PTR(-EBUSY);
+ } else { /* tty_init_dev returns tty with the tty_lock held */
+ tty = tty_init_dev(driver, index);
+ tty_port_set_kopened(tty->port, 1);
+ }
+out:
+ mutex_unlock(&tty_mutex);
+ tty_driver_kref_put(driver);
+ return tty;
+}
+EXPORT_SYMBOL_GPL(tty_kopen);
+
+/**
* tty_open_by_driver - open a tty device
* @device: dev_t of device to open
* @inode: inode of device file
@@ -1824,6 +1875,12 @@ struct tty_struct *tty_open_by_driver(de
}
if (tty) {
+ if (tty_port_kopened(tty->port)) {
+ tty_kref_put(tty);
+ mutex_unlock(&tty_mutex);
+ tty = ERR_PTR(-EBUSY);
+ goto out;
+ }
mutex_unlock(&tty_mutex);
retval = tty_lock_interruptible(tty);
tty_kref_put(tty); /* drop kref from tty_driver_lookup_tty() */
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -261,6 +261,7 @@ struct tty_port {
*/
#define TTY_PORT_CTS_FLOW 3 /* h/w flow control enabled */
#define TTY_PORT_CHECK_CD 4 /* carrier detect enabled */
+
[patch v2 2/3] staging: speakup: use tty_kopen instead of tty_open_by_driver
This patch replaces call to tty_open_by_driver with a tty_kopen. Signed-off-by: Okash Khawaja --- drivers/staging/speakup/spk_ttyio.c |2 +- 1 file changed, 1 insertion(+), 1 deletion(-) --- a/drivers/staging/speakup/spk_ttyio.c +++ b/drivers/staging/speakup/spk_ttyio.c @@ -158,7 +158,7 @@ static int spk_ttyio_initialise_ldisc(st if (ret) return ret; - tty = tty_open_by_driver(dev, NULL, NULL); + tty = tty_kopen(dev); if (IS_ERR(tty)) return PTR_ERR(tty);
[patch v2 1/3] tty: resolve tty contention between kernel and user space
The commit 12e84c71b7d4ee (tty: export tty_open_by_driver) exports
tty_open_by_device to allow tty to be opened from inside kernel which
works fine except that it doesn't handle contention with user space or
another kernel-space open of the same tty. For example, opening a tty
from user space while it is kernel opened results in failure and a
kernel log message about mismatch between tty->count and tty's file
open count.
This patch makes kernel access to tty exclusive, so that if a user
process or kernel opens a kernel opened tty, it gets -EBUSY. It does
this by adding TTY_KOPENED flag to tty->flags. When this flag is set,
tty_open_by_driver returns -EBUSY. Instead of overlaoding
tty_open_by_driver for both kernel and user space, this
patch creates a separate function tty_kopen which closely follows
tty_open_by_driver.
To address the mismatch between tty->count and #fd's, this patch adds
#kopen's to the count before comparing it with tty->count. That way
check_tty_count reflects correct usage count.
Returning -EBUSY on tty open is a change in the interface. I have
tested this with minicom, picocom and commands like "echo foo >
/dev/ttyS0". They all correctly report "Device or resource busy" when
the tty is already kernel opened.
Signed-off-by: Okash Khawaja
---
drivers/tty/tty_io.c | 66 +++
include/linux/tty.h |4 +++
2 files changed, 65 insertions(+), 5 deletions(-)
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -280,7 +280,7 @@ static int check_tty_count(struct tty_st
{
#ifdef CHECK_TTY_COUNT
struct list_head *p;
- int count = 0;
+ int count = 0, kopen_count = 0;
spin_lock(&tty->files_lock);
list_for_each(p, &tty->tty_files) {
@@ -291,10 +291,12 @@ static int check_tty_count(struct tty_st
tty->driver->subtype == PTY_TYPE_SLAVE &&
tty->link && tty->link->count)
count++;
- if (tty->count != count) {
- tty_warn(tty, "%s: tty->count(%d) != #fd's(%d)\n",
-routine, tty->count, count);
- return count;
+ if (test_bit(TTY_KOPENED, &tty->flags))
+ kopen_count++;
+ if (tty->count != (count + kopen_count)) {
+ tty_warn(tty, "%s: tty->count(%d) != (#fd's(%d) +
#kopen's(%d)\n",
+routine, tty->count, count, kopen_count);
+ return (count + kopen_count);
}
#endif
return 0;
@@ -1786,6 +1788,54 @@ static struct tty_driver *tty_lookup_dri
}
/**
+ * tty_kopen - open a tty device for kernel
+ * @device: dev_t of device to open
+ *
+ * Opens tty exclusively for kernel. Performs the driver lookup,
+ * makes sure it's not already opened and performs the first-time
+ * tty initialization.
+ *
+ * Returns the locked initialized &tty_struct
+ *
+ * Claims the global tty_mutex to serialize:
+ * - concurrent first-time tty initialization
+ * - concurrent tty driver removal w/ lookup
+ * - concurrent tty removal from driver table
+ */
+struct tty_struct *tty_kopen(dev_t device)
+{
+ struct tty_struct *tty;
+ struct tty_driver *driver = NULL;
+ int index = -1;
+
+ mutex_lock(&tty_mutex);
+ driver = tty_lookup_driver(device, NULL, &index);
+ if (IS_ERR(driver)) {
+ mutex_unlock(&tty_mutex);
+ return ERR_CAST(driver);
+ }
+
+ /* check whether we're reopening an existing tty */
+ tty = tty_driver_lookup_tty(driver, NULL, index);
+ if (IS_ERR(tty))
+ goto out;
+
+ if (tty) {
+ /* drop kref from tty_driver_lookup_tty() */
+ tty_kref_put(tty);
+ tty = ERR_PTR(-EBUSY);
+ } else { /* tty_init_dev returns tty with the tty_lock held */
+ tty = tty_init_dev(driver, index);
+ set_bit(TTY_KOPENED, &tty->flags);
+ }
+out:
+ mutex_unlock(&tty_mutex);
+ tty_driver_kref_put(driver);
+ return tty;
+}
+EXPORT_SYMBOL_GPL(tty_kopen);
+
+/**
* tty_open_by_driver - open a tty device
* @device: dev_t of device to open
* @inode: inode of device file
@@ -1824,6 +1874,12 @@ struct tty_struct *tty_open_by_driver(de
}
if (tty) {
+ if (test_bit(TTY_KOPENED, &tty->flags)) {
+ tty_kref_put(tty);
+ mutex_unlock(&tty_mutex);
+ tty = ERR_PTR(-EBUSY);
+ goto out;
+ }
mutex_unlock(&tty_mutex);
retval = tty_lock_interruptible(tty);
tty_kref_put(tty); /* drop kref from tty_driver_look
[patch v2 0/3] tty contention resulting from tty_open_by_driver export
Hi, I have reworked the previous patch set. These are the changes: 1. Patch 1 fixes tty->count mismatch reported by check_tty_count when a tty is kopened. 2. Patch 1 incorporates patch 4 in the previous patch set - it returns -ENODEV when tty is not configured. Thanks, Okash
[patch v2 3/3] tty: undo export of tty_open_by_driver
Since we have tty_kopen, we no longer need to export tty_open_by_driver.
This patch makes this function static.
Signed-off-by: Okash Khawaja
---
drivers/tty/tty_io.c |3 +--
include/linux/tty.h |5 -
2 files changed, 1 insertion(+), 7 deletions(-)
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -1851,7 +1851,7 @@ EXPORT_SYMBOL_GPL(tty_kopen);
* - concurrent tty driver removal w/ lookup
* - concurrent tty removal from driver table
*/
-struct tty_struct *tty_open_by_driver(dev_t device, struct inode *inode,
+static struct tty_struct *tty_open_by_driver(dev_t device, struct inode *inode,
struct file *filp)
{
struct tty_struct *tty;
@@ -1902,7 +1902,6 @@ out:
tty_driver_kref_put(driver);
return tty;
}
-EXPORT_SYMBOL_GPL(tty_open_by_driver);
/**
* tty_open- open a tty device
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -400,8 +400,6 @@ extern struct tty_struct *get_current_tt
/* tty_io.c */
extern int __init tty_init(void);
extern const char *tty_name(const struct tty_struct *tty);
-extern struct tty_struct *tty_open_by_driver(dev_t device, struct inode *inode,
- struct file *filp);
extern struct tty_struct *tty_kopen(dev_t device);
extern int tty_dev_name_to_number(const char *name, dev_t *number);
#else
@@ -424,9 +422,6 @@ static inline int __init tty_init(void)
{ return 0; }
static inline const char *tty_name(const struct tty_struct *tty)
{ return "(none)"; }
-static inline struct tty_struct *tty_open_by_driver(dev_t device,
- struct inode *inode, struct file *filp)
-{ return NULL; }
static inline struct tty_struct *tty_kopen(dev_t device)
{ return ERR_PTR(-ENODEV); }
static inline int tty_dev_name_to_number(const char *name, dev_t *number)
Re: [patch 0/3] Re: tty contention resulting from tty_open_by_device export
> On 17 Jul 2017, at 13:31, Greg Kroah-Hartman > wrote: > >> On Thu, Jul 13, 2017 at 12:29:54PM +0100, Okash Khawaja wrote: >>> On Wed, Jul 12, 2017 at 07:20:28PM +0100, Alan Cox wrote: >>> >>>> When opening from kernel, we don't use file pointer. The count mismatch >>>> is between tty->count and #fd's. So opening from kernel leads to #fd's >>>> being less than tty->count. I thought this difference is relevant to >>>> user-space opening of tty, and not to kernel opening of tty. Can you >>>> suggest how to address this mismatch? >>> >>> Your kernel reference is the same as having a file open reference so I >>> think this actually needs addressing in the maths. In other words count >>> the number of kernel references and also add that into the test for >>> check_tty_count (kernel references + #fds == count). >>> >>> I'd really like to keep this right because that check has a long history >>> of catching really nasty race conditions in the tty code. The >>> open/close/hangup code is really fragile so worth the debugability. >> >> I see. Okay based this, check_tty_count can be easily updated to take >> into account kernel references. > > Ok, I'll drop this series from my "to-apply" queue and wait for you to > redo it. Sure. I can fix the tty->count mismatch based on Alan's suggestion. However I don't understand why the exclusivity flag should belong to tty_port and not tty_struct. It will be good to know why. Thanks, Okash
[patch 2/2] staging: speakup: safely register and unregister ldisc
This patch makes use of functions added in the previous patch. It
registers ldisc during init of main speakup module and unregisters it
during exit. It also removes the code to register ldisc every time a
synth module is loaded. This way we only register the ldisc once when
main speakup module is loaded. Since main speakup module is required by
all synth modules, it is only unloaded when all synths have been
unloaded. Therefore we unregister the ldisc once, when all speakup
related references to the ldisc have returned. In unlikely scenario of
something outside speakup using the ldisc, the ldisc refcount check in
tty_unregister_ldisc will ensure that it is not unregistered while in
use.
The function to register ldisc doesn't cause speakup init function to
fail. That is different from current behaviour where failure to register
ldisc results in failure to load the specific synth module. This is
because speakup module is also required by those synths which don't use
tty and ldisc. We don't want to prevent those modules from loading when
ldisc fails to register. The synth modules will correctly fail when
trying to set N_SPEAKUP to tty, if ldisc registrationi had failed.
Signed-off-by: Okash Khawaja
---
drivers/staging/speakup/main.c |2 ++
drivers/staging/speakup/spk_ttyio.c |8 ++--
2 files changed, 4 insertions(+), 6 deletions(-)
--- a/drivers/staging/speakup/main.c
+++ b/drivers/staging/speakup/main.c
@@ -2315,6 +2315,7 @@ static void __exit speakup_exit(void)
mutex_lock(&spk_mutex);
synth_release();
mutex_unlock(&spk_mutex);
+ spk_ttyio_unregister_ldisc();
speakup_kobj_exit();
@@ -2377,6 +2378,7 @@ static int __init speakup_init(void)
if (err)
goto error_kobjects;
+ spk_ttyio_register_ldisc();
synth_init(synth_name);
speakup_register_devsynth();
/*
--- a/drivers/staging/speakup/spk_ttyio.c
+++ b/drivers/staging/speakup/spk_ttyio.c
@@ -154,12 +154,6 @@ static int spk_ttyio_initialise_ldisc(st
struct ktermios tmp_termios;
dev_t dev;
- ret = tty_register_ldisc(N_SPEAKUP, &spk_ttyio_ldisc_ops);
- if (ret) {
- pr_err("Error registering line discipline.\n");
- return ret;
- }
-
ret = get_dev_to_use(synth, &dev);
if (ret)
return ret;
@@ -196,6 +190,8 @@ static int spk_ttyio_initialise_ldisc(st
tty_unlock(tty);
ret = tty_set_ldisc(tty, N_SPEAKUP);
+ if (ret)
+ pr_err("speakup: Failed to set N_SPEAKUP on tty\n");
return ret;
}
[patch 0/2] staging: speakup: safely unregister ldisc
Hi, These patches make sure that N_SPEAKUP is correctly unregistered when all speakup related modules are unloaded, making sure the refcount correctly represents the number of users. Patch 1: simply adds functions to register and unregister ldisc, without changing existing behaviour Patch 2: uses those functions to register and unregister ldisc correctly Thanks, Okash
[patch 1/2] staging: speakup: add functions to register and unregister ldisc
This patch adds the above two functions and makes them available to
main.c where they will be called during init and exit functions of
main speakup module. Following patch will make use of them.
Signed-off-by: Okash Khawaja
---
drivers/staging/speakup/spk_priv.h |2 ++
drivers/staging/speakup/spk_ttyio.c | 12
2 files changed, 14 insertions(+)
--- a/drivers/staging/speakup/spk_priv.h
+++ b/drivers/staging/speakup/spk_priv.h
@@ -48,6 +48,8 @@ void spk_stop_serial_interrupt(void);
int spk_wait_for_xmitr(struct spk_synth *in_synth);
void spk_serial_release(void);
void spk_ttyio_release(void);
+void spk_ttyio_register_ldisc(void);
+void spk_ttyio_unregister_ldisc(void);
void synth_buffer_skip_nonlatin1(void);
u16 synth_buffer_getc(void);
--- a/drivers/staging/speakup/spk_ttyio.c
+++ b/drivers/staging/speakup/spk_ttyio.c
@@ -200,6 +200,18 @@ static int spk_ttyio_initialise_ldisc(st
return ret;
}
+void spk_ttyio_register_ldisc(void)
+{
+ if (tty_register_ldisc(N_SPEAKUP, &spk_ttyio_ldisc_ops))
+ pr_warn("speakup: Error registering line discipline. Most
synths won't work.\n");
+}
+
+void spk_ttyio_unregister_ldisc(void)
+{
+ if (tty_unregister_ldisc(N_SPEAKUP))
+ pr_warn("speakup: Couldn't unregister ldisc\n");
+}
+
static int spk_ttyio_out(struct spk_synth *in_synth, const char ch)
{
if (in_synth->alive && speakup_tty && speakup_tty->ops->write) {
[patch v2 0/1] staging: speakup: safely close tty
Hi, Let's deal with the ldisc refcount problem separately. Purpose of this patch is to close tty safely, so I have removed the call to unregister the ldisc. I will follow this up with a separate patch which addresses the ldisc issue. Thanks, Okash
[patch v2 1/1] staging: speakup: safely close tty
Speakup opens tty using tty_open_by_driver. When closing, it calls tty_ldisc_release but doesn't close and remove the tty itself. As a result, that tty cannot be opened from user space. This patch calls tty_release_struct which ensures that tty is safely removed and freed up. It also calls tty_ldisc_release, so speakup doesn't need to call it. Signed-off-by: Okash Khawaja --- drivers/staging/speakup/spk_ttyio.c |2 +- 1 file changed, 1 insertion(+), 1 deletion(-) --- a/drivers/staging/speakup/spk_ttyio.c +++ b/drivers/staging/speakup/spk_ttyio.c @@ -300,7 +300,7 @@ void spk_ttyio_release(void) tty_ldisc_flush(speakup_tty); tty_unlock(speakup_tty); - tty_ldisc_release(speakup_tty); + tty_release_struct(speakup_tty, speakup_tty->index); } EXPORT_SYMBOL_GPL(spk_ttyio_release);
Re: [patch 0/3] Re: tty contention resulting from tty_open_by_device export
On Wed, Jul 12, 2017 at 07:20:28PM +0100, Alan Cox wrote: > > > When opening from kernel, we don't use file pointer. The count mismatch > > is between tty->count and #fd's. So opening from kernel leads to #fd's > > being less than tty->count. I thought this difference is relevant to > > user-space opening of tty, and not to kernel opening of tty. Can you > > suggest how to address this mismatch? > > Your kernel reference is the same as having a file open reference so I > think this actually needs addressing in the maths. In other words count > the number of kernel references and also add that into the test for > check_tty_count (kernel references + #fds == count). > > I'd really like to keep this right because that check has a long history > of catching really nasty race conditions in the tty code. The > open/close/hangup code is really fragile so worth the debugability. I see. Okay based this, check_tty_count can be easily updated to take into account kernel references. > > > Ah may be I didn't notice the active bit. Is it one of the #defines in > > tty.h? Can usage count and active bit be used to differentiate between > > whether the tty was opened by kernel or user? > > It only tells you whether the port is currently active for some purpose, > not which. If you still want to implement exclusivity between kernel and > user then it needs another flag, but I think that flag should be in > port->flags as it is a property of the physical interface. > > (Take a look at tty_port_open for example) Okay I can add TTY_PORT_KOPENED to port->flags and that should work too. However, can you please help me understand this: Our use case requires kernel access to tty_struct and accordingly tty_kopen returns tty_struct. The exclusivity between user and kernel space is also meant to prevent one side from opening tty_struct while another has it opened. In all this, it is tty_struct and not tty_port which is the key resource we are concerned with. So shouldn't the exclusivity flag belong to tty_struct? Adding a the flag to port->flags but controlling it from code for opening and closing tty will also mean we have tty_port_kopened, tty_port_set_kopen etc inside tty open/close code. Am I viewing this problem incorrectly? Thanks, Okash
Re: [patch] staging: speakup: safely close tty
On Wed, Jul 12, 2017 at 07:25:22PM +0100, Alan Cox wrote: > > spk_ttyio_initialise_ldisc is called separately for each module (e.g. > > speakup_apollo, speakup_ltlk etc) when it is loaded. spk_ttyio_release > > is also called separately for each module when it is unloaded. The ldisc > > stays around until the last of the modules is unloaded. > > What guarantees that someone hasn't decided to set the ldisc on unrelated > hardware to speakup (eg on a pty/tty pair). > > > > > > > > I'd also btw strongly recommend putting the ldisc and the speakup tty > > > driver as different modules. > > Sure, that makes sense. I will do that following these patches. > > If the ldisc is just unregistered when the module implementing it is > unloaded then the ref counts on the ldisc module should do everything > needed if the above isn't correctly handled, and if it is will still be > cleaner. Right, I understand now. Thanks. I will update and resend this patch.
Re: [patch] staging: speakup: safely close tty
Hi, On Mon, Jul 10, 2017 at 12:55:44PM +0100, Alan Cox wrote: > On Fri, 7 Jul 2017 20:13:01 +0100 > Okash Khawaja wrote: > > > Speakup opens tty using tty_open_by_driver. When closing, it calls > > tty_ldisc_release but doesn't close and remove the tty itself. As a > > result, that tty cannot then be opened from user space. This patch calls > > tty_release_struct which ensures that tty is safely removed and freed > > up. It also calls tty_ldisc_release, so speakup doesn't need to call it. > > > > This patch also unregisters N_SPEAKUP. It is registered when a speakup > > module is loaded. > > What happens if after you register it someone assigns that ldisc to > another tty as well ? > > You should register the ldisc when the relevant module is initialized and > release it only when the module is unloaded. That way the module ref > counts will handle cases where someone uses the ldisc with something else. Sorry if I misunderstood it. That's what we do here. spk_ttyio_initialise_ldisc is called separately for each module (e.g. speakup_apollo, speakup_ltlk etc) when it is loaded. spk_ttyio_release is also called separately for each module when it is unloaded. The ldisc stays around until the last of the modules is unloaded. > > I'd also btw strongly recommend putting the ldisc and the speakup tty > driver as different modules. Sure, that makes sense. I will do that following these patches. Thanks, Okash
Re: [patch 0/3] Re: tty contention resulting from tty_open_by_device export
On Mon, Jul 10, 2017 at 01:33:07PM +0100, Okash Khawaja wrote: > > If the tty counts are being misreported then it would be better to fix > > the code to actually manage the counts properly. The core tty code is > > telling you that the tty is not in a valid state. While this is of > > itself a good API to have, the underlying reference miscounting ought > > IMHO to be fixed as well. > When opening from kernel, we don't use file pointer. The count mismatch > is between tty->count and #fd's. So opening from kernel leads to #fd's > being less than tty->count. I thought this difference is relevant to > user-space opening of tty, and not to kernel opening of tty. Can you > suggest how to address this mismatch? Idea is tty_kopen only ever returns a newly initialised tty - returned by tty_init_dev. Since the access is exclusive, tty->count shouldn't matter. When the caller is done with it, it calls tty_release_struct which frees up the tty itself. But yes, all the while a tty is kopened, its tty->count and #fds will be unequal. Thanks, Okash
Re: [patch 1/3] tty: resolve tty contention between kernel and user space
On Mon, Jul 10, 2017 at 06:21:37PM +0300, Andy Shevchenko wrote:
> On Mon, Jul 10, 2017 at 11:31 AM, Okash Khawaja
> wrote:
> > On Sun, Jul 09, 2017 at 06:04:17PM +0300, Andy Shevchenko wrote:
> >> On Sun, Jul 9, 2017 at 2:41 PM, Okash Khawaja
> >> wrote:
> >>
> >> > +struct tty_struct *tty_kopen(dev_t device)
> >> > +{
> >> > + struct tty_struct *tty;
> >> > + struct tty_driver *driver = NULL;
> >> > + int index = -1;
> >> > +
> >> > + mutex_lock(&tty_mutex);
> >> > + driver = tty_lookup_driver(device, NULL, &index);
> >> > + if (IS_ERR(driver)) {
> >>
> >> > + mutex_unlock(&tty_mutex);
> >> > + return ERR_CAST(driver);
> >>
> >> Hmm... perhaps
> >>
> >> tty = ERR_CAST(driver);
> >> goto out_unlock;
> >>
> >> See below for further details.
> >>
> > Sorry missed this one out. Since tty_lookup_driver has failed, we don't
> > need to down the refcount on driver. So we can return here, without
> > going to out_unlock.
>
> Yeah, and my point is to use goto with the symmetric giveups of lock
> and reference.
Ah okay I see your point. Sure, I don't mind either way. However, this
code closely follows tty_open_by_driver, so I have tried to keep the
same pattern for sake of consistency :)
Thanks,
Okash
Re: [patch 0/3] Re: tty contention resulting from tty_open_by_device export
On Mon, Jul 10, 2017 at 12:52:33PM +0100, Alan Cox wrote: > On Sun, 09 Jul 2017 12:41:53 +0100 > Okash Khawaja wrote: > > > On Sat, Jul 08, 2017 at 10:38:03AM +0200, Greg Kroah-Hartman wrote: > > > Overall, the idea looks sane to me. Keeping userspace from opening a > > > tty that the kernel has opened internally makes sense, hopefully > > > userspace doesn't get too confused when that happens. I don't think we > > > normally return -EBUSY from an open call, have you seen what happens > > > with apps when you do this (like minicom?) > > > > > I tested this wil minincom, picocom and commands like "echo foo > > > /dev/ttyS0". They all correctly report "Device or resource busy". > > > > I have addressed all the comments you made. I have also split the patch > > into three. Following is summary of each. > > If the tty counts are being misreported then it would be better to fix > the code to actually manage the counts properly. The core tty code is > telling you that the tty is not in a valid state. While this is of > itself a good API to have, the underlying reference miscounting ought > IMHO to be fixed as well. When opening from kernel, we don't use file pointer. The count mismatch is between tty->count and #fd's. So opening from kernel leads to #fd's being less than tty->count. I thought this difference is relevant to user-space opening of tty, and not to kernel opening of tty. Can you suggest how to address this mismatch? > > Also you don't need a new TTY_KOPENED flag as far as I can see. All tty's > have a usage count and active bit flags already. Use those. Ah may be I didn't notice the active bit. Is it one of the #defines in tty.h? Can usage count and active bit be used to differentiate between whether the tty was opened by kernel or user? Thanks, Okash
Re: [patch 1/3] tty: resolve tty contention between kernel and user space
On Sun, Jul 09, 2017 at 06:04:17PM +0300, Andy Shevchenko wrote:
> On Sun, Jul 9, 2017 at 2:41 PM, Okash Khawaja wrote:
>
> > +struct tty_struct *tty_kopen(dev_t device)
> > +{
> > + struct tty_struct *tty;
> > + struct tty_driver *driver = NULL;
> > + int index = -1;
> > +
> > + mutex_lock(&tty_mutex);
> > + driver = tty_lookup_driver(device, NULL, &index);
> > + if (IS_ERR(driver)) {
>
> > + mutex_unlock(&tty_mutex);
> > + return ERR_CAST(driver);
>
> Hmm... perhaps
>
> tty = ERR_CAST(driver);
> goto out_unlock;
>
> See below for further details.
>
Sorry missed this one out. Since tty_lookup_driver has failed, we don't
need to down the refcount on driver. So we can return here, without
going to out_unlock.
Thanks,
Okash
Re: [patch 1/3] tty: resolve tty contention between kernel and user space
Hi, On Sun, Jul 09, 2017 at 06:04:17PM +0300, Andy Shevchenko wrote: > Above > 1. take mutex > 2. take reference > > Here: > 1. give mutex back > 2. give reference back > > I think we usually see symmetrical calls, i.e. > > 1. give reference back > 2. give mutex back > > So, what did I miss? After we hold the kref, driver won't disappear from underneath us so we don't need tty_mutex just for decrementing the refcount. Thanks, Okash
[patch 4/3] tty: make tty_kopen return ENODEV in case of no TTY
When TTY is not built, tty_kopen should return an error. This way
calling code remains consistent, regardless of whether tty is built or
not. This patch returns -ENODEV when there is no tty.
Signed-off-by: Okash Khawaja
---
include/linux/tty.h |2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -423,7 +423,7 @@ static inline int __init tty_init(void)
static inline const char *tty_name(const struct tty_struct *tty)
{ return "(none)"; }
static inline struct tty_struct *tty_kopen(dev_t device)
-{ return NULL; }
+{ return ERR_PTR(-ENODEV); }
static inline int tty_dev_name_to_number(const char *name, dev_t *number)
{ return -ENOTSUPP; }
#endif
Re: [patch 2/3] staging: speakup: use tty_kopen instead of tty_open_by_driver
On Sun, Jul 09, 2017 at 01:50:55PM +0200, Greg Kroah-Hartman wrote: > On Sun, Jul 09, 2017 at 12:41:55PM +0100, Okash Khawaja wrote: > > This patch replaces call to tty_open_by_driver with a tty_kopen. > > > > Signed-off-by: Okash Khawaja > > > > --- > > drivers/staging/speakup/spk_ttyio.c |2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > --- a/drivers/staging/speakup/spk_ttyio.c > > +++ b/drivers/staging/speakup/spk_ttyio.c > > @@ -164,7 +164,7 @@ static int spk_ttyio_initialise_ldisc(st > > if (ret) > > return ret; > > > > - tty = tty_open_by_driver(dev, NULL, NULL); > > + tty = tty_kopen(dev); > > if (IS_ERR(tty)) > > Hm, the "no tty layer" return value for this will be NULL. I doubt > that's really a big deal, but perhaps just have that return > PTR_ERR(-ENODEV) or something? Good point, thanks. Will send a follow up patch Okash
[patch 1/3] tty: resolve tty contention between kernel and user space
The commit 12e84c71b7d4ee (tty: export tty_open_by_driver) exports
tty_open_by_device to allow tty to be opened from inside kernel which
works fine except that it doesn't handle contention with user space or
another kernel-space open of the same tty. For example, opening a tty
from user space while it is kernel opened results in failure and a
kernel log message about mismatch between tty->count and tty's file
open count.
This patch makes kernel access to tty exclusive, so that if a user
process or kernel opens a kernel opened tty, it gets -EBUSY. It does
this by adding TTY_KOPENED flag to tty->flags. When this flag is set,
tty_open_by_driver returns -EBUSY. Instead of overlaoding
tty_open_by_driver for both kernel and user space, this
patch creates a separate function tty_kopen which closely follows
tty_open_by_driver.
Returning -EBUSY on tty open is a change in the interface. I have
tested this with minicom, picocom and commands like "echo foo >
/dev/ttyS0". They all correctly report "Device or resource busy" when
the tty is already kernel opened.
Signed-off-by: Okash Khawaja
---
drivers/tty/tty_io.c | 54 +++
include/linux/tty.h |4 +++
2 files changed, 58 insertions(+)
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -1786,6 +1786,54 @@ static struct tty_driver *tty_lookup_dri
}
/**
+ * tty_kopen - open a tty device for kernel
+ * @device: dev_t of device to open
+ *
+ * Opens tty exclusively for kernel. Performs the driver lookup,
+ * makes sure it's not already opened and performs the first-time
+ * tty initialization.
+ *
+ * Returns the locked initialized &tty_struct
+ *
+ * Claims the global tty_mutex to serialize:
+ * - concurrent first-time tty initialization
+ * - concurrent tty driver removal w/ lookup
+ * - concurrent tty removal from driver table
+ */
+struct tty_struct *tty_kopen(dev_t device)
+{
+ struct tty_struct *tty;
+ struct tty_driver *driver = NULL;
+ int index = -1;
+
+ mutex_lock(&tty_mutex);
+ driver = tty_lookup_driver(device, NULL, &index);
+ if (IS_ERR(driver)) {
+ mutex_unlock(&tty_mutex);
+ return ERR_CAST(driver);
+ }
+
+ /* check whether we're reopening an existing tty */
+ tty = tty_driver_lookup_tty(driver, NULL, index);
+ if (IS_ERR(tty))
+ goto out;
+
+ if (tty) {
+ /* drop kref from tty_driver_lookup_tty() */
+ tty_kref_put(tty);
+ tty = ERR_PTR(-EBUSY);
+ } else { /* tty_init_dev returns tty with the tty_lock held */
+ tty = tty_init_dev(driver, index);
+ set_bit(TTY_KOPENED, &tty->flags);
+ }
+out:
+ mutex_unlock(&tty_mutex);
+ tty_driver_kref_put(driver);
+ return tty;
+}
+EXPORT_SYMBOL_GPL(tty_kopen);
+
+/**
* tty_open_by_driver - open a tty device
* @device: dev_t of device to open
* @inode: inode of device file
@@ -1824,6 +1872,12 @@ struct tty_struct *tty_open_by_driver(de
}
if (tty) {
+ if (test_bit(TTY_KOPENED, &tty->flags)) {
+ tty_kref_put(tty);
+ mutex_unlock(&tty_mutex);
+ tty = ERR_PTR(-EBUSY);
+ goto out;
+ }
mutex_unlock(&tty_mutex);
retval = tty_lock_interruptible(tty);
tty_kref_put(tty); /* drop kref from tty_driver_lookup_tty() */
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -362,6 +362,7 @@ struct tty_file_private {
#define TTY_NO_WRITE_SPLIT 17 /* Preserve write boundaries to driver
*/
#define TTY_HUPPED 18 /* Post driver->hangup() */
#define TTY_LDISC_HALTED 22 /* Line discipline is halted */
+#define TTY_KOPENED23 /* TTY exclusively opened by kernel */
/* Values for tty->flow_change */
#define TTY_THROTTLE_SAFE 1
@@ -401,6 +402,7 @@ extern int __init tty_init(void);
extern const char *tty_name(const struct tty_struct *tty);
extern struct tty_struct *tty_open_by_driver(dev_t device, struct inode *inode,
struct file *filp);
+extern struct tty_struct *tty_kopen(dev_t device);
extern int tty_dev_name_to_number(const char *name, dev_t *number);
#else
static inline void tty_kref_put(struct tty_struct *tty)
@@ -425,6 +427,8 @@ static inline const char *tty_name(const
static inline struct tty_struct *tty_open_by_driver(dev_t device,
struct inode *inode, struct file *filp)
{ return NULL; }
+static inline struct tty_struct *tty_kopen(dev_t device)
+{ return NULL; }
static inline int tty_dev_name_to_number(const char *name, dev_t *number)
{ return -ENOTSUPP; }
#endif
[patch 3/3] tty: undo export of tty_open_by_driver
Since we have tty_kopen, we no longer need to export tty_open_by_driver.
This patch makes this function static.
Signed-off-by: Okash Khawaja
---
drivers/tty/tty_io.c |3 +--
include/linux/tty.h |5 -
2 files changed, 1 insertion(+), 7 deletions(-)
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -1849,7 +1849,7 @@ EXPORT_SYMBOL_GPL(tty_kopen);
* - concurrent tty driver removal w/ lookup
* - concurrent tty removal from driver table
*/
-struct tty_struct *tty_open_by_driver(dev_t device, struct inode *inode,
+static struct tty_struct *tty_open_by_driver(dev_t device, struct inode *inode,
struct file *filp)
{
struct tty_struct *tty;
@@ -1900,7 +1900,6 @@ out:
tty_driver_kref_put(driver);
return tty;
}
-EXPORT_SYMBOL_GPL(tty_open_by_driver);
/**
* tty_open- open a tty device
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -400,8 +400,6 @@ extern struct tty_struct *get_current_tt
/* tty_io.c */
extern int __init tty_init(void);
extern const char *tty_name(const struct tty_struct *tty);
-extern struct tty_struct *tty_open_by_driver(dev_t device, struct inode *inode,
- struct file *filp);
extern struct tty_struct *tty_kopen(dev_t device);
extern int tty_dev_name_to_number(const char *name, dev_t *number);
#else
@@ -424,9 +422,6 @@ static inline int __init tty_init(void)
{ return 0; }
static inline const char *tty_name(const struct tty_struct *tty)
{ return "(none)"; }
-static inline struct tty_struct *tty_open_by_driver(dev_t device,
- struct inode *inode, struct file *filp)
-{ return NULL; }
static inline struct tty_struct *tty_kopen(dev_t device)
{ return NULL; }
static inline int tty_dev_name_to_number(const char *name, dev_t *number)
[patch 0/3] Re: tty contention resulting from tty_open_by_device export
On Sat, Jul 08, 2017 at 10:38:03AM +0200, Greg Kroah-Hartman wrote: > Overall, the idea looks sane to me. Keeping userspace from opening a > tty that the kernel has opened internally makes sense, hopefully > userspace doesn't get too confused when that happens. I don't think we > normally return -EBUSY from an open call, have you seen what happens > with apps when you do this (like minicom?) > I tested this wil minincom, picocom and commands like "echo foo > /dev/ttyS0". They all correctly report "Device or resource busy". I have addressed all the comments you made. I have also split the patch into three. Following is summary of each. Patch 1: introduces the tty_kopen function and checks for TTY_KOPENED Patch 2: updates speakup code to use tty_kopen instead of tty_open_by_driver Patch 3: reverses the export of tty_open_by_driver Thanks, Okash
[patch 2/3] staging: speakup: use tty_kopen instead of tty_open_by_driver
This patch replaces call to tty_open_by_driver with a tty_kopen. Signed-off-by: Okash Khawaja --- drivers/staging/speakup/spk_ttyio.c |2 +- 1 file changed, 1 insertion(+), 1 deletion(-) --- a/drivers/staging/speakup/spk_ttyio.c +++ b/drivers/staging/speakup/spk_ttyio.c @@ -164,7 +164,7 @@ static int spk_ttyio_initialise_ldisc(st if (ret) return ret; - tty = tty_open_by_driver(dev, NULL, NULL); + tty = tty_kopen(dev); if (IS_ERR(tty)) return PTR_ERR(tty);
Re: tty contention resulting from tty_open_by_device export
On Sat, Jul 08, 2017 at 10:38:03AM +0200, Greg Kroah-Hartman wrote:
> > +
> > +if (tty) {
> > +tty_kref_put(tty); /* drop kref from
> > tty_driver_lookup_tty() */
>
> Put the comment above the line?
>
Sure
> > +tty = ERR_PTR(-EBUSY);
> > +} else { /* Returns with the tty_lock held for now */
>
> I don't understand this comment.
>
This is same comment in tty_open_by_driver. Basically, tty_init_dev
returns tty locked so that it doesn't dissapear from underneath the
caller. I am not sure about the "for now" part of the comment. I'll
remove it.
> > +tty = tty_init_dev(driver, index);
> > +set_bit(TTY_KOPENED, &tty->flags);
> > +}
> > +out:
> > +mutex_unlock(&tty_mutex);
> > +tty_driver_kref_put(driver);
> > +return tty;
> > +}
> > +EXPORT_SYMBOL(tty_kopen);
>
> EXPORT_SYMBOL_GPL()? :)
>
Of course, will update
> > /**
> > * tty_open- open a tty device
> > --- a/include/linux/tty.h
> > +++ b/include/linux/tty.h
> > @@ -362,6 +362,7 @@ struct tty_file_private {
> > #define TTY_NO_WRITE_SPLIT 17 /* Preserve write boundaries to
> > driver */
> > #define TTY_HUPPED 18 /* Post driver->hangup() */
> > #define TTY_LDISC_HALTED 22 /* Line discipline is halted */
> > +#define TTY_KOPENED 23 /* TTY exclusively opened by
> > kernel */
>
> Minor nit, please use tabs here.
>
Will do
> Overall, the idea looks sane to me. Keeping userspace from opening a
> tty that the kernel has opened internally makes sense, hopefully
> userspace doesn't get too confused when that happens. I don't think we
> normally return -EBUSY from an open call, have you seen what happens
> with apps when you do this (like minicom?)
>
Yes, returning -EBUSY is a change in the interface. I will test against
applications like minicom before resubmitting this.
Thanks,
Okash
tty contention resulting from tty_open_by_device export
Hi,
The commit 12e84c71b7d4ee (tty: export tty_open_by_driver) exports
tty_open_by_device to allow tty to be opened from inside kernel which
works fine except that it doesn't handle contention with user space or
another kernel-space open of the same tty. For example, opening a tty
from user space while it is kernel opened results in failure and a
kernel log message about mismatch between tty->count and tty's file open
count.
I suggest we make kernel access to tty exclusive so that if a user
process or kernel opens a kernel opened tty, it gets -EBUSY. Below is
a patch which does this by adding TTY_KOPENED flag to tty->flags. When
this flag is set, tty_open_by_driver returns -EBUSY. Instead of
overlaoding tty_open_by_driver for both kernel and user space, this
patch creates a separate function tty_kopen which closely follows
tty_open_by_driver.
I am far from an expert on tty and this patch might contain the wrong
approach. But it should convey what I mean.
Thanks,
Okash
---
drivers/staging/speakup/spk_ttyio.c |2 -
drivers/tty/tty_io.c| 56 ++--
include/linux/tty.h |7 +---
3 files changed, 58 insertions(+), 7 deletions(-)
--- a/drivers/staging/speakup/spk_ttyio.c
+++ b/drivers/staging/speakup/spk_ttyio.c
@@ -164,7 +164,7 @@ static int spk_ttyio_initialise_ldisc(st
if (ret)
return ret;
- tty = tty_open_by_driver(dev, NULL, NULL);
+ tty = tty_kopen(dev);
if (IS_ERR(tty))
return PTR_ERR(tty);
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -1786,6 +1786,53 @@ static struct tty_driver *tty_lookup_dri
}
/**
+ * tty_kopen - open a tty device for kernel
+ * @device: dev_t of device to open
+ *
+ * Opens tty exclusively for kernel. Performs the driver lookup,
+ * makes sure it's not already opened and performs the first-time
+ * tty initialization.
+ *
+ * Returns the locked initialized &tty_struct
+ *
+ * Claims the global tty_mutex to serialize:
+ * - concurrent first-time tty initialization
+ * - concurrent tty driver removal w/ lookup
+ * - concurrent tty removal from driver table
+ */
+struct tty_struct *tty_kopen(dev_t device)
+{
+struct tty_struct *tty;
+struct tty_driver *driver = NULL;
+int index = -1;
+
+mutex_lock(&tty_mutex);
+driver = tty_lookup_driver(device, NULL, &index);
+if (IS_ERR(driver)) {
+mutex_unlock(&tty_mutex);
+return ERR_CAST(driver);
+}
+
+/* check whether we're reopening an existing tty */
+tty = tty_driver_lookup_tty(driver, NULL, index);
+if (IS_ERR(tty))
+goto out;
+
+if (tty) {
+tty_kref_put(tty); /* drop kref from tty_driver_lookup_tty()
*/
+tty = ERR_PTR(-EBUSY);
+} else { /* Returns with the tty_lock held for now */
+tty = tty_init_dev(driver, index);
+set_bit(TTY_KOPENED, &tty->flags);
+}
+out:
+mutex_unlock(&tty_mutex);
+tty_driver_kref_put(driver);
+return tty;
+}
+EXPORT_SYMBOL(tty_kopen);
+
+/**
* tty_open_by_driver - open a tty device
* @device: dev_t of device to open
* @inode: inode of device file
@@ -1801,7 +1848,7 @@ static struct tty_driver *tty_lookup_dri
* - concurrent tty driver removal w/ lookup
* - concurrent tty removal from driver table
*/
-struct tty_struct *tty_open_by_driver(dev_t device, struct inode *inode,
+static struct tty_struct *tty_open_by_driver(dev_t device, struct inode *inode,
struct file *filp)
{
struct tty_struct *tty;
@@ -1824,6 +1871,12 @@ struct tty_struct *tty_open_by_driver(de
}
if (tty) {
+ if (test_bit(TTY_KOPENED, &tty->flags)) {
+ tty_kref_put(tty);
+ mutex_unlock(&tty_mutex);
+ tty = ERR_PTR(-EBUSY);
+ goto out;
+ }
mutex_unlock(&tty_mutex);
retval = tty_lock_interruptible(tty);
tty_kref_put(tty); /* drop kref from tty_driver_lookup_tty() */
@@ -1846,7 +1899,6 @@ out:
tty_driver_kref_put(driver);
return tty;
}
-EXPORT_SYMBOL_GPL(tty_open_by_driver);
/**
* tty_open- open a tty device
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -362,6 +362,7 @@ struct tty_file_private {
#define TTY_NO_WRITE_SPLIT 17 /* Preserve write boundaries to driver
*/
#define TTY_HUPPED 18 /* Post driver->hangup() */
#define TTY_LDISC_HALTED 22 /* Line discipline is halted */
+#define TTY_KOPENED 23 /* TTY exclusively opened by kernel */
/* Values for tty->flow_change */
#define TTY_THROTTLE_SAFE 1
@@ -399,8 +400,7 @@
[patch] staging: speakup: safely close tty
Speakup opens tty using tty_open_by_driver. When closing, it calls
tty_ldisc_release but doesn't close and remove the tty itself. As a
result, that tty cannot then be opened from user space. This patch calls
tty_release_struct which ensures that tty is safely removed and freed
up. It also calls tty_ldisc_release, so speakup doesn't need to call it.
This patch also unregisters N_SPEAKUP. It is registered when a speakup
module is loaded.
Signed-off-by: Okash Khawaja
---
drivers/staging/speakup/spk_ttyio.c |4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
--- a/drivers/staging/speakup/spk_ttyio.c
+++ b/drivers/staging/speakup/spk_ttyio.c
@@ -300,7 +300,9 @@ void spk_ttyio_release(void)
tty_ldisc_flush(speakup_tty);
tty_unlock(speakup_tty);
- tty_ldisc_release(speakup_tty);
+ tty_release_struct(speakup_tty, speakup_tty->index);
+ if (tty_unregister_ldisc(N_SPEAKUP))
+ pr_warn("speakup: failed to unregister line discipline
N_SPEAKUP\n");
}
EXPORT_SYMBOL_GPL(spk_ttyio_release);
Re: [PATCH] staging: speakup: make function ser_to_dev static
Hi,
On Wed, Jun 28, 2017 at 02:13:51PM +0100, Colin King wrote:
> From: Colin Ian King
>
> The helper function ser_to_dev does not need to be in global scope, so
> make it static.
>
> Cleans up sparse warning:
> "warning: symbol 'ser_to_dev' was not declared. Should it be static?"
Thanks very much!
>
> Signed-off-by: Colin Ian King
Reviewed-by: Okash Khawaja
> ---
> drivers/staging/speakup/spk_ttyio.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/staging/speakup/spk_ttyio.c
> b/drivers/staging/speakup/spk_ttyio.c
> index 442f191a017e..ed8e96b06ead 100644
> --- a/drivers/staging/speakup/spk_ttyio.c
> +++ b/drivers/staging/speakup/spk_ttyio.c
> @@ -21,7 +21,7 @@ struct spk_ldisc_data {
> static struct spk_synth *spk_ttyio_synth;
> static struct tty_struct *speakup_tty;
>
> -int ser_to_dev(int ser, dev_t *dev_no)
> +static int ser_to_dev(int ser, dev_t *dev_no)
> {
> if (ser < 0 || ser > (255 - 64)) {
> pr_err("speakup: Invalid ser param. Must be between 0 and 191
> inclusive.\n");
> --
> 2.11.0
>
[patch v4 1/3] tty: add function to convert device name to number
The function converts strings like ttyS0 and ttyUSB0 to dev_t like
(4, 64) and (188, 0). It does this by scanning tty_drivers list for
corresponding device name and index. If the driver is not registered,
this function returns -ENODEV. It also acquires tty_mutex.
Signed-off-by: Okash Khawaja
---
drivers/tty/tty_io.c | 50 ++
include/linux/tty.h |3 +++
2 files changed, 53 insertions(+)
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -325,6 +325,56 @@ static struct tty_driver *get_tty_driver
return NULL;
}
+/**
+ * tty_dev_name_to_number - return dev_t for device name
+ * @name: user space name of device under /dev
+ * @number: pointer to dev_t that this function will populate
+ *
+ * This function converts device names like ttyS0 or ttyUSB1 into dev_t
+ * like (4, 64) or (188, 1). If no corresponding driver is registered then
+ * the function returns -ENODEV.
+ *
+ * Locking: this acquires tty_mutex to protect the tty_drivers list from
+ * being modified while we are traversing it, and makes sure to
+ * release it before exiting.
+ */
+int tty_dev_name_to_number(const char *name, dev_t *number)
+{
+ struct tty_driver *p;
+ int ret;
+ int index, prefix_length = 0;
+ const char *str;
+
+ for (str = name; *str && !isdigit(*str); str++)
+ ;
+
+ if (!*str)
+ return -EINVAL;
+
+ ret = kstrtoint(str, 10, &index);
+ if (ret)
+ return ret;
+
+ prefix_length = str - name;
+ mutex_lock(&tty_mutex);
+
+ list_for_each_entry(p, &tty_drivers, tty_drivers)
+ if (prefix_length == strlen(p->name) && strncmp(name,
+ p->name, prefix_length) == 0) {
+ if (index < p->num) {
+ *number = MKDEV(p->major, p->minor_start +
index);
+ goto out;
+ }
+ }
+
+ /* if here then driver wasn't found */
+ ret = -ENODEV;
+out:
+ mutex_unlock(&tty_mutex);
+ return ret;
+}
+EXPORT_SYMBOL_GPL(tty_dev_name_to_number);
+
#ifdef CONFIG_CONSOLE_POLL
/**
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -401,6 +401,7 @@ extern int __init tty_init(void);
extern const char *tty_name(const struct tty_struct *tty);
extern struct tty_struct *tty_open_by_driver(dev_t device, struct inode *inode,
struct file *filp);
+extern int tty_dev_name_to_number(const char *name, dev_t *number);
#else
static inline void tty_kref_put(struct tty_struct *tty)
{ }
@@ -424,6 +425,8 @@ static inline const char *tty_name(const
static inline struct tty_struct *tty_open_by_driver(dev_t device,
struct inode *inode, struct file *filp)
{ return NULL; }
+static inline int tty_dev_name_to_number(const char *name, dev_t *number)
+{ return -ENOTSUPP; }
#endif
extern struct ktermios tty_std_termios;
[patch v4 3/3] staging: speakup: make ttyio synths use device name
This patch introduces new module parameter, dev, which takes a string
representing the device that the external synth is connected to, e.g.
ttyS0, ttyUSB0 etc. This is then used to communicate with the synth.
That way, speakup can support more than ttyS*. As of this patch, it
only supports ttyS*, ttyUSB* and selected synths for lp*. dev parameter
is only available for tty-migrated synths.
Users will either use dev or ser as both serve same purpose. This patch
maintains backward compatility by allowing ser to be specified. When
both are specified, whichever is non-default, i.e. not ttyS0, is used.
If both are non-default then dev is used.
Signed-off-by: Okash Khawaja
Reviewed-by: Samuel Thibault
Reviewed-by: Andy Shevchenko
---
drivers/staging/speakup/speakup_acntsa.c |3 +++
drivers/staging/speakup/speakup_apollo.c |3 +++
drivers/staging/speakup/speakup_audptr.c |3 +++
drivers/staging/speakup/speakup_bns.c|3 +++
drivers/staging/speakup/speakup_decext.c |3 +++
drivers/staging/speakup/speakup_dectlk.c |3 +++
drivers/staging/speakup/speakup_dummy.c |3 +++
drivers/staging/speakup/speakup_ltlk.c |3 +++
drivers/staging/speakup/speakup_spkout.c |3 +++
drivers/staging/speakup/speakup_txprt.c |3 +++
drivers/staging/speakup/spk_ttyio.c | 15 +++
11 files changed, 37 insertions(+), 8 deletions(-)
--- a/drivers/staging/speakup/speakup_acntsa.c
+++ b/drivers/staging/speakup/speakup_acntsa.c
@@ -96,6 +96,7 @@ static struct spk_synth synth_acntsa = {
.trigger = 50,
.jiffies = 30,
.full = 4,
+ .dev_name = SYNTH_DEFAULT_DEV,
.startup = SYNTH_START,
.checkval = SYNTH_CHECK,
.vars = vars,
@@ -135,9 +136,11 @@ static int synth_probe(struct spk_synth
}
module_param_named(ser, synth_acntsa.ser, int, 0444);
+module_param_named(dev, synth_acntsa.dev_name, charp, S_IRUGO);
module_param_named(start, synth_acntsa.startup, short, 0444);
MODULE_PARM_DESC(ser, "Set the serial port for the synthesizer (0-based).");
+MODULE_PARM_DESC(dev, "Set the device e.g. ttyUSB0, for the synthesizer.");
MODULE_PARM_DESC(start, "Start the synthesizer once it is loaded.");
module_spk_synth(synth_acntsa);
--- a/drivers/staging/speakup/speakup_apollo.c
+++ b/drivers/staging/speakup/speakup_apollo.c
@@ -105,6 +105,7 @@ static struct spk_synth synth_apollo = {
.trigger = 50,
.jiffies = 50,
.full = 4,
+ .dev_name = SYNTH_DEFAULT_DEV,
.startup = SYNTH_START,
.checkval = SYNTH_CHECK,
.vars = vars,
@@ -199,9 +200,11 @@ static void do_catch_up(struct spk_synth
}
module_param_named(ser, synth_apollo.ser, int, 0444);
+module_param_named(dev, synth_apollo.dev_name, charp, S_IRUGO);
module_param_named(start, synth_apollo.startup, short, 0444);
MODULE_PARM_DESC(ser, "Set the serial port for the synthesizer (0-based).");
+MODULE_PARM_DESC(dev, "Set the device e.g. ttyUSB0, for the synthesizer.");
MODULE_PARM_DESC(start, "Start the synthesizer once it is loaded.");
module_spk_synth(synth_apollo);
--- a/drivers/staging/speakup/speakup_audptr.c
+++ b/drivers/staging/speakup/speakup_audptr.c
@@ -100,6 +100,7 @@ static struct spk_synth synth_audptr = {
.trigger = 50,
.jiffies = 30,
.full = 18000,
+ .dev_name = SYNTH_DEFAULT_DEV,
.startup = SYNTH_START,
.checkval = SYNTH_CHECK,
.vars = vars,
@@ -162,9 +163,11 @@ static int synth_probe(struct spk_synth
}
module_param_named(ser, synth_audptr.ser, int, 0444);
+module_param_named(dev, synth_audptr.dev_name, charp, S_IRUGO);
module_param_named(start, synth_audptr.startup, short, 0444);
MODULE_PARM_DESC(ser, "Set the serial port for the synthesizer (0-based).");
+MODULE_PARM_DESC(dev, "Set the device e.g. ttyUSB0, for the synthesizer.");
MODULE_PARM_DESC(start, "Start the synthesizer once it is loaded.");
module_spk_synth(synth_audptr);
--- a/drivers/staging/speakup/speakup_bns.c
+++ b/drivers/staging/speakup/speakup_bns.c
@@ -93,6 +93,7 @@ static struct spk_synth synth_bns = {
.trigger = 50,
.jiffies = 50,
.full = 4,
+ .dev_name = SYNTH_DEFAULT_DEV,
.startup = SYNTH_START,
.checkval = SYNTH_CHECK,
.vars = vars,
@@ -119,9 +120,11 @@ static struct spk_synth synth_bns = {
};
module_param_named(ser, synth_bns.ser, int, 0444);
+module_param_named(dev, synth_bns.dev_name, charp, S_IRUGO);
module_param_named(start, synth_bns.startup, short, 0444);
MODULE_PARM_DESC(ser, "Set the serial port for the synthesizer (0-based).");
+MODULE_PARM_DESC(dev, "Set the device e.g. ttyUSB0, for the synthesizer.");
MODULE_PARM_DESC(start, "Start the synthesizer once it is loaded.");
module_spk_synth(synth_bns);
--- a/drivers/staging/speakup/speakup_decext.c
+++ b/dri
[patch v4 2/3] staging: speakup: check and convert dev name or ser to dev_t
This patch adds functionality to validate and convert either a device
name or 'ser' memmber of synth into dev_t. Subsequent patch in this set
will call it to convert user-specified device into device number. For
device name, this patch does some basic sanity checks on the string
passed in. It currently supports ttyS*, ttyUSB* and, for selected
synths, lp*.
The patch also introduces a string member variable named 'dev_name' to
struct spk_synth. 'dev_name' represents the device name - ttyUSB0 etc -
which needs conversion to dev_t.
Signed-off-by: Okash Khawaja
Reviewed-by: Andy Shevchenko
---
drivers/staging/speakup/spk_priv.h |2 +
drivers/staging/speakup/spk_ttyio.c | 42
drivers/staging/speakup/spk_types.h |1
3 files changed, 45 insertions(+)
--- a/drivers/staging/speakup/spk_priv.h
+++ b/drivers/staging/speakup/spk_priv.h
@@ -40,6 +40,8 @@
#define KT_SPKUP 15
#define SPK_SYNTH_TIMEOUT 10 /* in micro-seconds */
+#define SYNTH_DEFAULT_DEV "ttyS0"
+#define SYNTH_DEFAULT_SER 0
const struct old_serial_port *spk_serial_init(int index);
void spk_stop_serial_interrupt(void);
--- a/drivers/staging/speakup/spk_ttyio.c
+++ b/drivers/staging/speakup/spk_ttyio.c
@@ -7,6 +7,11 @@
#include "spk_types.h"
#include "spk_priv.h"
+#define DEV_PREFIX_LP "lp"
+
+static const char * const lp_supported[] = { "acntsa", "bns", "dummy",
+ "txprt" };
+
struct spk_ldisc_data {
char buf;
struct semaphore sem;
@@ -16,6 +21,43 @@ struct spk_ldisc_data {
static struct spk_synth *spk_ttyio_synth;
static struct tty_struct *speakup_tty;
+int ser_to_dev(int ser, dev_t *dev_no)
+{
+ if (ser < 0 || ser > (255 - 64)) {
+ pr_err("speakup: Invalid ser param. Must be between 0 and 191
inclusive.\n");
+ return -EINVAL;
+ }
+
+ *dev_no = MKDEV(4, (64 + ser));
+ return 0;
+}
+
+static int get_dev_to_use(struct spk_synth *synth, dev_t *dev_no)
+{
+ /* use ser only when dev is not specified */
+ if (strcmp(synth->dev_name, SYNTH_DEFAULT_DEV) ||
+ synth->ser == SYNTH_DEFAULT_SER) {
+ /* for /dev/lp* check if synth is supported */
+ if (strncmp(synth->dev_name, DEV_PREFIX_LP,
+ strlen(DEV_PREFIX_LP)) == 0)
+ if (match_string(lp_supported, ARRAY_SIZE(lp_supported),
+ synth->name) < 0) {
+ int i;
+
+ pr_err("speakup: lp* is only supported on:");
+ for (i = 0; i < ARRAY_SIZE(lp_supported); i++)
+ pr_cont(" %s", lp_supported[i]);
+ pr_cont("\n");
+
+ return -ENOTSUPP;
+ }
+
+ return tty_dev_name_to_number(synth->dev_name, dev_no);
+ }
+
+ return ser_to_dev(synth->ser, dev_no);
+}
+
static int spk_ttyio_ldisc_open(struct tty_struct *tty)
{
struct spk_ldisc_data *ldisc_data;
--- a/drivers/staging/speakup/spk_types.h
+++ b/drivers/staging/speakup/spk_types.h
@@ -169,6 +169,7 @@ struct spk_synth {
int jiffies;
int full;
int ser;
+ char *dev_name;
short flags;
short startup;
const int checkval; /* for validating a proper synth module */
[patch v4 0/3] staging: speakup: support more than ttyS*
Hi, v3 of this patch set introduced the compiler warning: drivers//tty/tty_io.c:348:11: warning: assignment discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers] This version fixes that warning in patch 1/3. Thanks, Okash
[patch] staging: speakup: fix synth caching when synth init fails
synths[] array caches currently loaded synths. synth_add checks
synths[] before adding a new one. It however ignores the result of
do_synth_init. So when do_synth_init fails, the failed synth is still
cached. Since, as a result module loading fails too, synth_remove -
which is responsible for removing the cached synth - is never called.
Next time the failing synth is added again it succeeds because
synth_add finds it cached inside synths[].
This patch fixes this by caching a synth only after do_synth_init
succeeds.
Signed-off-by: Okash Khawaja
Reviewed-by: Samuel Thibault
---
drivers/staging/speakup/synth.c |9 +++--
1 file changed, 7 insertions(+), 2 deletions(-)
--- a/drivers/staging/speakup/synth.c
+++ b/drivers/staging/speakup/synth.c
@@ -445,10 +445,15 @@ int synth_add(struct spk_synth *in_synth
mutex_unlock(&spk_mutex);
return -1;
}
- synths[i++] = in_synth;
- synths[i] = NULL;
+
if (in_synth->startup)
status = do_synth_init(in_synth);
+
+ if (!status) {
+ synths[i++] = in_synth;
+ synths[i] = NULL;
+ }
+
mutex_unlock(&spk_mutex);
return status;
}
[patch v3 1/3] tty: add function to convert device name to number
The function converts strings like ttyS0 and ttyUSB0 to dev_t like
(4, 64) and (188, 0). It does this by scanning tty_drivers list for
corresponding device name and index. If the driver is not registered,
this function returns -ENODEV. It also acquires tty_mutex.
Signed-off-by: Okash Khawaja
---
drivers/tty/tty_io.c | 50 ++
include/linux/tty.h |3 +++
2 files changed, 53 insertions(+)
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -325,6 +325,56 @@ static struct tty_driver *get_tty_driver
return NULL;
}
+/**
+ * tty_dev_name_to_number - return dev_t for device name
+ * @name: user space name of device under /dev
+ * @number: pointer to dev_t that this function will populate
+ *
+ * This function converts device names like ttyS0 or ttyUSB1 into dev_t
+ * like (4, 64) or (188, 1). If no corresponding driver is registered then
+ * the function returns -ENODEV.
+ *
+ * Locking: this acquires tty_mutex to protect the tty_drivers list from
+ * being modified while we are traversing it, and makes sure to
+ * release it before exiting.
+ */
+int tty_dev_name_to_number(const char *name, dev_t *number)
+{
+ struct tty_driver *p;
+ int ret;
+ int index, prefix_length = 0;
+ char *str;
+
+ for (str = name; *str && !isdigit(*str); str++)
+ ;
+
+ if (!*str)
+ return -EINVAL;
+
+ ret = kstrtoint(str, 10, &index);
+ if (ret)
+ return ret;
+
+ prefix_length = str - name;
+ mutex_lock(&tty_mutex);
+
+ list_for_each_entry(p, &tty_drivers, tty_drivers)
+ if (prefix_length == strlen(p->name) && strncmp(name,
+ p->name, prefix_length) == 0) {
+ if (index < p->num) {
+ *number = MKDEV(p->major, p->minor_start +
index);
+ goto out;
+ }
+ }
+
+ /* if here then driver wasn't found */
+ ret = -ENODEV;
+out:
+ mutex_unlock(&tty_mutex);
+ return ret;
+}
+EXPORT_SYMBOL_GPL(tty_dev_name_to_number);
+
#ifdef CONFIG_CONSOLE_POLL
/**
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -401,6 +401,7 @@ extern int __init tty_init(void);
extern const char *tty_name(const struct tty_struct *tty);
extern struct tty_struct *tty_open_by_driver(dev_t device, struct inode *inode,
struct file *filp);
+extern int tty_dev_name_to_number(const char *name, dev_t *number);
#else
static inline void tty_kref_put(struct tty_struct *tty)
{ }
@@ -424,6 +425,8 @@ static inline const char *tty_name(const
static inline struct tty_struct *tty_open_by_driver(dev_t device,
struct inode *inode, struct file *filp)
{ return NULL; }
+static inline int tty_dev_name_to_number(const char *name, dev_t *number)
+{ return -ENOTSUPP; }
#endif
extern struct ktermios tty_std_termios;
[patch v3 3/3] staging: speakup: make ttyio synths use device name
This patch introduces new module parameter, dev, which takes a string
representing the device that the external synth is connected to, e.g.
ttyS0, ttyUSB0 etc. This is then used to communicate with the synth.
That way, speakup can support more than ttyS*. As of this patch, it
only supports ttyS*, ttyUSB* and selected synths for lp*. dev parameter
is only available for tty-migrated synths.
Users will either use dev or ser as both serve same purpose. This patch
maintains backward compatility by allowing ser to be specified. When
both are specified, whichever is non-default, i.e. not ttyS0, is used.
If both are non-default then dev is used.
Signed-off-by: Okash Khawaja
Reviewed-by: Samuel Thibault
Reviewed-by: Andy Shevchenko
---
drivers/staging/speakup/speakup_acntsa.c |3 +++
drivers/staging/speakup/speakup_apollo.c |3 +++
drivers/staging/speakup/speakup_audptr.c |3 +++
drivers/staging/speakup/speakup_bns.c|3 +++
drivers/staging/speakup/speakup_decext.c |3 +++
drivers/staging/speakup/speakup_dectlk.c |3 +++
drivers/staging/speakup/speakup_dummy.c |3 +++
drivers/staging/speakup/speakup_ltlk.c |3 +++
drivers/staging/speakup/speakup_spkout.c |3 +++
drivers/staging/speakup/speakup_txprt.c |3 +++
drivers/staging/speakup/spk_ttyio.c | 15 +++
11 files changed, 37 insertions(+), 8 deletions(-)
--- a/drivers/staging/speakup/speakup_acntsa.c
+++ b/drivers/staging/speakup/speakup_acntsa.c
@@ -96,6 +96,7 @@ static struct spk_synth synth_acntsa = {
.trigger = 50,
.jiffies = 30,
.full = 4,
+ .dev_name = SYNTH_DEFAULT_DEV,
.startup = SYNTH_START,
.checkval = SYNTH_CHECK,
.vars = vars,
@@ -135,9 +136,11 @@ static int synth_probe(struct spk_synth
}
module_param_named(ser, synth_acntsa.ser, int, 0444);
+module_param_named(dev, synth_acntsa.dev_name, charp, S_IRUGO);
module_param_named(start, synth_acntsa.startup, short, 0444);
MODULE_PARM_DESC(ser, "Set the serial port for the synthesizer (0-based).");
+MODULE_PARM_DESC(dev, "Set the device e.g. ttyUSB0, for the synthesizer.");
MODULE_PARM_DESC(start, "Start the synthesizer once it is loaded.");
module_spk_synth(synth_acntsa);
--- a/drivers/staging/speakup/speakup_apollo.c
+++ b/drivers/staging/speakup/speakup_apollo.c
@@ -105,6 +105,7 @@ static struct spk_synth synth_apollo = {
.trigger = 50,
.jiffies = 50,
.full = 4,
+ .dev_name = SYNTH_DEFAULT_DEV,
.startup = SYNTH_START,
.checkval = SYNTH_CHECK,
.vars = vars,
@@ -199,9 +200,11 @@ static void do_catch_up(struct spk_synth
}
module_param_named(ser, synth_apollo.ser, int, 0444);
+module_param_named(dev, synth_apollo.dev_name, charp, S_IRUGO);
module_param_named(start, synth_apollo.startup, short, 0444);
MODULE_PARM_DESC(ser, "Set the serial port for the synthesizer (0-based).");
+MODULE_PARM_DESC(dev, "Set the device e.g. ttyUSB0, for the synthesizer.");
MODULE_PARM_DESC(start, "Start the synthesizer once it is loaded.");
module_spk_synth(synth_apollo);
--- a/drivers/staging/speakup/speakup_audptr.c
+++ b/drivers/staging/speakup/speakup_audptr.c
@@ -100,6 +100,7 @@ static struct spk_synth synth_audptr = {
.trigger = 50,
.jiffies = 30,
.full = 18000,
+ .dev_name = SYNTH_DEFAULT_DEV,
.startup = SYNTH_START,
.checkval = SYNTH_CHECK,
.vars = vars,
@@ -162,9 +163,11 @@ static int synth_probe(struct spk_synth
}
module_param_named(ser, synth_audptr.ser, int, 0444);
+module_param_named(dev, synth_audptr.dev_name, charp, S_IRUGO);
module_param_named(start, synth_audptr.startup, short, 0444);
MODULE_PARM_DESC(ser, "Set the serial port for the synthesizer (0-based).");
+MODULE_PARM_DESC(dev, "Set the device e.g. ttyUSB0, for the synthesizer.");
MODULE_PARM_DESC(start, "Start the synthesizer once it is loaded.");
module_spk_synth(synth_audptr);
--- a/drivers/staging/speakup/speakup_bns.c
+++ b/drivers/staging/speakup/speakup_bns.c
@@ -93,6 +93,7 @@ static struct spk_synth synth_bns = {
.trigger = 50,
.jiffies = 50,
.full = 4,
+ .dev_name = SYNTH_DEFAULT_DEV,
.startup = SYNTH_START,
.checkval = SYNTH_CHECK,
.vars = vars,
@@ -119,9 +120,11 @@ static struct spk_synth synth_bns = {
};
module_param_named(ser, synth_bns.ser, int, 0444);
+module_param_named(dev, synth_bns.dev_name, charp, S_IRUGO);
module_param_named(start, synth_bns.startup, short, 0444);
MODULE_PARM_DESC(ser, "Set the serial port for the synthesizer (0-based).");
+MODULE_PARM_DESC(dev, "Set the device e.g. ttyUSB0, for the synthesizer.");
MODULE_PARM_DESC(start, "Start the synthesizer once it is loaded.");
module_spk_synth(synth_bns);
--- a/drivers/staging/speakup/speakup_decext.c
+++ b/dri
[patch v3 0/3] staging: speakup: support more than ttyS*
Hi, I have updated the patches based on feedback. For patch 1, In favour of consistency, I've updated the code which extracts trailing digits so that it is like similar code in tty_find_polling_driver. Also fixed checkpatch warnings. Here's summary of the patches Patch 1 adds functionality to convert device name into dev_t. Patch 2 uses the function in patch 1 and performs some checks. Patch 3 adds new module param and actually extends support to more than ttyS*. Thanks, Okash
[patch v3 2/3] staging: speakup: check and convert dev name or ser to dev_t
This patch adds functionality to validate and convert either a device
name or 'ser' memmber of synth into dev_t. Subsequent patch in this set
will call it to convert user-specified device into device number. For
device name, this patch does some basic sanity checks on the string
passed in. It currently supports ttyS*, ttyUSB* and, for selected
synths, lp*.
The patch also introduces a string member variable named 'dev_name' to
struct spk_synth. 'dev_name' represents the device name - ttyUSB0 etc -
which needs conversion to dev_t.
Signed-off-by: Okash Khawaja
Reviewed-by: Andy Shevchenko
---
drivers/staging/speakup/spk_priv.h |2 +
drivers/staging/speakup/spk_ttyio.c | 42
drivers/staging/speakup/spk_types.h |1
3 files changed, 45 insertions(+)
--- a/drivers/staging/speakup/spk_priv.h
+++ b/drivers/staging/speakup/spk_priv.h
@@ -40,6 +40,8 @@
#define KT_SPKUP 15
#define SPK_SYNTH_TIMEOUT 10 /* in micro-seconds */
+#define SYNTH_DEFAULT_DEV "ttyS0"
+#define SYNTH_DEFAULT_SER 0
const struct old_serial_port *spk_serial_init(int index);
void spk_stop_serial_interrupt(void);
--- a/drivers/staging/speakup/spk_ttyio.c
+++ b/drivers/staging/speakup/spk_ttyio.c
@@ -7,6 +7,11 @@
#include "spk_types.h"
#include "spk_priv.h"
+#define DEV_PREFIX_LP "lp"
+
+static const char * const lp_supported[] = { "acntsa", "bns", "dummy",
+ "txprt" };
+
struct spk_ldisc_data {
char buf;
struct semaphore sem;
@@ -16,6 +21,43 @@ struct spk_ldisc_data {
static struct spk_synth *spk_ttyio_synth;
static struct tty_struct *speakup_tty;
+int ser_to_dev(int ser, dev_t *dev_no)
+{
+ if (ser < 0 || ser > (255 - 64)) {
+ pr_err("speakup: Invalid ser param. Must be between 0 and 191
inclusive.\n");
+ return -EINVAL;
+ }
+
+ *dev_no = MKDEV(4, (64 + ser));
+ return 0;
+}
+
+static int get_dev_to_use(struct spk_synth *synth, dev_t *dev_no)
+{
+ /* use ser only when dev is not specified */
+ if (strcmp(synth->dev_name, SYNTH_DEFAULT_DEV) ||
+ synth->ser == SYNTH_DEFAULT_SER) {
+ /* for /dev/lp* check if synth is supported */
+ if (strncmp(synth->dev_name, DEV_PREFIX_LP,
+ strlen(DEV_PREFIX_LP)) == 0)
+ if (match_string(lp_supported, ARRAY_SIZE(lp_supported),
+ synth->name) < 0) {
+ int i;
+
+ pr_err("speakup: lp* is only supported on:");
+ for (i = 0; i < ARRAY_SIZE(lp_supported); i++)
+ pr_cont(" %s", lp_supported[i]);
+ pr_cont("\n");
+
+ return -ENOTSUPP;
+ }
+
+ return tty_dev_name_to_number(synth->dev_name, dev_no);
+ }
+
+ return ser_to_dev(synth->ser, dev_no);
+}
+
static int spk_ttyio_ldisc_open(struct tty_struct *tty)
{
struct spk_ldisc_data *ldisc_data;
--- a/drivers/staging/speakup/spk_types.h
+++ b/drivers/staging/speakup/spk_types.h
@@ -169,6 +169,7 @@ struct spk_synth {
int jiffies;
int full;
int ser;
+ char *dev_name;
short flags;
short startup;
const int checkval; /* for validating a proper synth module */
Re: [patch v2 1/3] tty: add function to convert device name to number
On Sun, Jun 18, 2017 at 04:27:42PM +0300, Andy Shevchenko wrote:
> This doesn't have actual parameter name.
> Btw, I would drop dev_ suffix completely from parameter (you have it
> in function name).
Good call, thanks.
> > + * Locking: this acquires tty_mutex
>
> ...and releases.
>
> Perhaps it makes sense to describe what it protects (like it's done in
> some functions around).
Yes, will add the description
> > + */
> > +int tty_dev_name_to_number(char *dev_name, dev_t *dev_no)
>
> const char *name, right?
Yes :)
> > + int rv, index, prefix_length = 0;
>
> I would keep returned variable on a separate line and name it like
> other functions do in this file, i.e. ret.
>
> int ret;
Sure
> > + while (!isdigit(*(dev_name + prefix_length)) && prefix_length <
> > + strlen(dev_name) )
> > + prefix_length++;
> > +
> > + if (prefix_length == strlen(dev_name))
> > + return -EINVAL;
>
> Basically, what you need is to get tailing digits, right?
>
> Moreover, there is quite similar piece of code in
> tty_find_polling_driver() you may share.
tty_find_polling_driver does something slightly different. It looks for
digits embedded in a string, so something like kgdboc=ttyS0,115200 where
the digit being sought is 0 after ttyS. So the sanity checks aren't the
same. It also looks for a comma in addition to looking for a number,
which doesn't apply here. Little bit of functionality may be factored
out but that will be too trivial.
While skimming through tty_find_polling_driver, I also noticed that, in
a strict sense, the following check may not be sufficient when name is
prefix of p->name.
if (strncmp(name, p->name, len) != 0)
> > + if (rv) {
>
> > + mutex_unlock(&tty_mutex);
> > + return rv;
>
> I would go with goto style in this function (since it has locking involved).
Sure
>
> > + }
>
> All together kstrtoint() is invariant here as far as I can see and can
> be done out of locking. Also see above comment how to get line index.
Good call, thanks. Think I changed the code afterwards but didn't
notice that kstrtoint no longer needs to be inside the loop and lock.
Best regards,
Okash
Re: [patch v2 2/3] staging: speakup: check and convert dev name or ser to dev_t
On Mon, Jun 19, 2017 at 09:15:33AM +0800, Greg Kroah-Hartman wrote:
> > +int ser_to_dev(int ser, dev_t *dev_no)
> > +{
> > + if (ser < 0 || ser > (255 - 64)) {
> > +pr_err("speakup: Invalid ser param. \
> > + Must be between 0 and 191 inclusive.\n");
>
> As Andy pointed out, never do this for a C string, it's not doing what
> you think it is :)
Of course! I am sorry I should address such issues before submitting.
Will watch out more carefully next time.
>
> Worse case, do this like the following:
> pr_err("speakup: Invalid ser param."
> "Must be between 0 and 191 inclusive.\n");
>
> Also note, you are using spaces here in the patch, always run
> checkpatch.pl on your patches, so you don't get a grumpy maintainer
> telling you to run checkpatch.pl on your patches :)
Sure.
Thanks for the patience.
Okash
Re: [patch v2 2/3] staging: speakup: check and convert dev name or ser to dev_t
Hi,
Thanks for the reviews. Couple of things inlined below.
On Sun, Jun 18, 2017 at 04:35:21PM +0300, Andy Shevchenko wrote:
>
> > +const char *lp_supported[] = { "acntsa", "bns", "dummy", "txprt" };
>
> static ?
Sure!
> > + if (ser < 0 || ser > (255 - 64)) {
>
> > +pr_err("speakup: Invalid ser param. \
> > + Must be between 0 and 191 inclusive.\n");
>
> Just make it one line.
Is it okay if it becomes larger than 80 chars?
> > +
> > + for (i = 0; i < ARRAY_SIZE(lp_supported); i++) {
> > + if (strcmp(synth->name, lp_supported[i]) ==
> > 0)
> > + break;
> > + }
> > +
> > + if (i >= ARRAY_SIZE(lp_supported)) {
>
> match_string()
Cool, didn't know about it
>
> > + pr_err("speakup: lp* is only supported
> > on:");
>
> > + for (i = 0; i < ARRAY_SIZE(lp_supported);
> > i++)
> > + pr_cont(" %s", lp_supported[i]);
> > + pr_cont("\n");
>
> pr_cont() is not the best idea, though I think it will be rare cases
> when it might be broken in pieces.
Hmm... I would like to keep it if it doesn't incur an overhead. It also
indicates to the reader that this all part of same output line. Let me
know what you think.
>
> > +
> > + return -ENOTSUPP;
> > + }
> > + }
> > +
> > + return tty_dev_name_to_number(synth->dev_name, dev_no);
> > + }
> > +
> > + return ser_to_dev(synth->ser, dev_no);
> > +}
> > +
> > static int spk_ttyio_ldisc_open(struct tty_struct *tty)
> > {
> > struct spk_ldisc_data *ldisc_data;
> > --- a/drivers/staging/speakup/spk_types.h
> > +++ b/drivers/staging/speakup/spk_types.h
> > @@ -169,6 +169,7 @@ struct spk_synth {
> > int jiffies;
> > int full;
> > int ser;
>
> > + char *dev_name;
>
> const ?
This becomes the target of module_param in following patch. It complains
when set to const.
Thanks!
Okash
[patch v2 2/3] staging: speakup: check and convert dev name or ser to dev_t
This patch adds functionality to validate and convert either a device
name or 'ser' member of synth into dev_t. Subsequent patch in this set
will call it to convert user-specified device into device number. For
device name, this patch does some basic sanity checks on the string
passed in. It currently supports ttyS*, ttyUSB* and, for selected
synths, lp*.
The patch also introduces a string member variable named 'dev_name' to
struct spk_synth. 'dev_name' represents the device name - ttyUSB0 etc -
which needs conversion to dev_t.
Signed-off-by: Okash Khawaja
---
drivers/staging/speakup/spk_priv.h |2 +
drivers/staging/speakup/spk_ttyio.c | 46
drivers/staging/speakup/spk_types.h |1
3 files changed, 49 insertions(+)
--- a/drivers/staging/speakup/spk_priv.h
+++ b/drivers/staging/speakup/spk_priv.h
@@ -40,6 +40,8 @@
#define KT_SPKUP 15
#define SPK_SYNTH_TIMEOUT 10 /* in micro-seconds */
+#define SYNTH_DEFAULT_DEV "ttyS0"
+#define SYNTH_DEFAULT_SER 0
const struct old_serial_port *spk_serial_init(int index);
void spk_stop_serial_interrupt(void);
--- a/drivers/staging/speakup/spk_ttyio.c
+++ b/drivers/staging/speakup/spk_ttyio.c
@@ -7,6 +7,10 @@
#include "spk_types.h"
#include "spk_priv.h"
+#define DEV_PREFIX_LP "lp"
+
+const char *lp_supported[] = { "acntsa", "bns", "dummy", "txprt" };
+
struct spk_ldisc_data {
char buf;
struct semaphore sem;
@@ -16,6 +20,48 @@ struct spk_ldisc_data {
static struct spk_synth *spk_ttyio_synth;
static struct tty_struct *speakup_tty;
+int ser_to_dev(int ser, dev_t *dev_no)
+{
+ if (ser < 0 || ser > (255 - 64)) {
+pr_err("speakup: Invalid ser param. \
+ Must be between 0 and 191 inclusive.\n");
+
+ return -EINVAL;
+}
+
+ *dev_no = MKDEV(4, (64 + ser));
+ return 0;
+}
+
+static int get_dev_to_use(struct spk_synth *synth, dev_t *dev_no)
+{
+ /* use ser only when dev is not specified */
+ if (strcmp(synth->dev_name, SYNTH_DEFAULT_DEV) || synth->ser ==
SYNTH_DEFAULT_SER) {
+ /* for /dev/lp* check if synth is supported */
+ if (strncmp(synth->dev_name, DEV_PREFIX_LP,
strlen(DEV_PREFIX_LP)) == 0) {
+ int i;
+
+ for (i = 0; i < ARRAY_SIZE(lp_supported); i++) {
+ if (strcmp(synth->name, lp_supported[i]) == 0)
+ break;
+ }
+
+ if (i >= ARRAY_SIZE(lp_supported)) {
+ pr_err("speakup: lp* is only supported on:");
+ for (i = 0; i < ARRAY_SIZE(lp_supported); i++)
+ pr_cont(" %s", lp_supported[i]);
+ pr_cont("\n");
+
+ return -ENOTSUPP;
+ }
+ }
+
+ return tty_dev_name_to_number(synth->dev_name, dev_no);
+ }
+
+ return ser_to_dev(synth->ser, dev_no);
+}
+
static int spk_ttyio_ldisc_open(struct tty_struct *tty)
{
struct spk_ldisc_data *ldisc_data;
--- a/drivers/staging/speakup/spk_types.h
+++ b/drivers/staging/speakup/spk_types.h
@@ -169,6 +169,7 @@ struct spk_synth {
int jiffies;
int full;
int ser;
+ char *dev_name;
short flags;
short startup;
const int checkval; /* for validating a proper synth module */
[patch v2 0/3] staging: speakup: support more than ttyS*
Hi, The patchset now contains a separate patch for dev name-to-number conversion functionality inside tty_io.c. These patches extend speakup support to ttyUSB* and lp*. They introduce a new module param dev whose function is similar to ser but instead of taking serial port number as argument, it takes strings like ttyS0 or ttyUSB0. Patch 1 adds functionality to convert such strings into dev_t. Patch 2 uses the function in patch 1 and performs some checks. Patch 3 adds new module param and actually extends support to more than ttyS*. Samuel, I have kept the Reviewed-by for patch 3 which has slightly changed from before - 'dev' renamed to 'dev_name' - but not for patch 2 which is a bit different now that dev name to number conversion has moved to tty_io.c. Please let me know if you think otherwise. Thanks, Okash
[patch v2 1/3] tty: add function to convert device name to number
The function converts strings like ttyS0 and ttyUSB0 to dev_t like
(4, 64) and (188, 0). It does this by scanning tty_drivers list for
corresponding device name and index. If the driver is not registered,
this function returns -ENODEV. It also acquires tty_mutex.
Signed-off-by: Okash Khawaja
---
drivers/tty/tty_io.c | 45 +
include/linux/tty.h |4
2 files changed, 49 insertions(+)
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -325,6 +325,51 @@ static struct tty_driver *get_tty_driver
return NULL;
}
+/**
+ * tty_dev_name_to_number - return dev_t for device name
+ * @device_name: user space name of device under /dev
+ * @dev_no: pointer to dev_t that this function will populate
+ *
+ * This function converts device names like ttyS0 or ttyUSB1 into dev_t
+ * like (4, 64) or (188, 1). If no corresponding driver is registered then
+ * the function returns -ENODEV.
+ *
+ * Locking: this acquires tty_mutex
+ */
+int tty_dev_name_to_number(char *dev_name, dev_t *dev_no)
+{
+ struct tty_driver *p;
+ int rv, index, prefix_length = 0;
+
+ while (!isdigit(*(dev_name + prefix_length)) && prefix_length <
+ strlen(dev_name) )
+ prefix_length++;
+
+ if (prefix_length == strlen(dev_name))
+ return -EINVAL;
+
+ mutex_lock(&tty_mutex);
+
+ list_for_each_entry(p, &tty_drivers, tty_drivers)
+ if (prefix_length == strlen(p->name) && strncmp(dev_name,
+ p->name, prefix_length) == 0) {
+ rv = kstrtoint(dev_name + prefix_length, 10, &index);
+ if (rv) {
+ mutex_unlock(&tty_mutex);
+ return rv;
+ }
+ if (index < p->num) {
+ *dev_no = MKDEV(p->major, p->minor_start +
index);
+ mutex_unlock(&tty_mutex);
+ return 0;
+ }
+ }
+
+ mutex_unlock(&tty_mutex);
+ return -ENODEV;
+}
+EXPORT_SYMBOL_GPL(tty_dev_name_to_number);
+
#ifdef CONFIG_CONSOLE_POLL
/**
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -401,6 +401,7 @@ extern int __init tty_init(void);
extern const char *tty_name(const struct tty_struct *tty);
extern struct tty_struct *tty_open_by_driver(dev_t device, struct inode *inode,
struct file *filp);
+extern int tty_dev_name_to_number(char *dev_name, dev_t *dev_no);
#else
static inline void tty_kref_put(struct tty_struct *tty)
{ }
@@ -424,6 +425,9 @@ static inline const char *tty_name(const
static inline struct tty_struct *tty_open_by_driver(dev_t device,
struct inode *inode, struct file *filp)
{ return NULL; }
+static inline int tty_dev_name_to_number(char *dev_name, dev_t *dev_no)
+{ return -ENOTSUPP; }
+
#endif
extern struct ktermios tty_std_termios;
[patch v2 3/3] staging: speakup: make ttyio synths use device name
This patch introduces new module parameter, dev, which takes a string
representing the device that the external synth is connected to, e.g.
ttyS0, ttyUSB0 etc. This is then used to communicate with the synth.
That way, speakup can support more than ttyS*. As of this patch, it
only supports ttyS*, ttyUSB* and selected synths for lp*. dev parameter
is only available for tty-migrated synths.
Users will either use dev or ser as both serve same purpose. This patch
maintains backward compatility by allowing ser to be specified. When
both are specified, whichever is non-default, i.e. not ttyS0, is used.
If both are non-default then dev is used.
Signed-off-by: Okash Khawaja
Reviewed-by: Samuel Thibault
---
drivers/staging/speakup/speakup_acntsa.c |3 +++
drivers/staging/speakup/speakup_apollo.c |3 +++
drivers/staging/speakup/speakup_audptr.c |3 +++
drivers/staging/speakup/speakup_bns.c|3 +++
drivers/staging/speakup/speakup_decext.c |3 +++
drivers/staging/speakup/speakup_dectlk.c |3 +++
drivers/staging/speakup/speakup_dummy.c |3 +++
drivers/staging/speakup/speakup_ltlk.c |3 +++
drivers/staging/speakup/speakup_spkout.c |3 +++
drivers/staging/speakup/speakup_txprt.c |3 +++
drivers/staging/speakup/spk_ttyio.c | 15 +++
11 files changed, 37 insertions(+), 8 deletions(-)
--- a/drivers/staging/speakup/speakup_acntsa.c
+++ b/drivers/staging/speakup/speakup_acntsa.c
@@ -96,6 +96,7 @@ static struct spk_synth synth_acntsa = {
.trigger = 50,
.jiffies = 30,
.full = 4,
+ .dev_name = SYNTH_DEFAULT_DEV,
.startup = SYNTH_START,
.checkval = SYNTH_CHECK,
.vars = vars,
@@ -135,9 +136,11 @@ static int synth_probe(struct spk_synth
}
module_param_named(ser, synth_acntsa.ser, int, 0444);
+module_param_named(dev, synth_acntsa.dev_name, charp, S_IRUGO);
module_param_named(start, synth_acntsa.startup, short, 0444);
MODULE_PARM_DESC(ser, "Set the serial port for the synthesizer (0-based).");
+MODULE_PARM_DESC(dev, "Set the device e.g. ttyUSB0, for the synthesizer.");
MODULE_PARM_DESC(start, "Start the synthesizer once it is loaded.");
module_spk_synth(synth_acntsa);
--- a/drivers/staging/speakup/speakup_apollo.c
+++ b/drivers/staging/speakup/speakup_apollo.c
@@ -105,6 +105,7 @@ static struct spk_synth synth_apollo = {
.trigger = 50,
.jiffies = 50,
.full = 4,
+ .dev_name = SYNTH_DEFAULT_DEV,
.startup = SYNTH_START,
.checkval = SYNTH_CHECK,
.vars = vars,
@@ -199,9 +200,11 @@ static void do_catch_up(struct spk_synth
}
module_param_named(ser, synth_apollo.ser, int, 0444);
+module_param_named(dev, synth_apollo.dev_name, charp, S_IRUGO);
module_param_named(start, synth_apollo.startup, short, 0444);
MODULE_PARM_DESC(ser, "Set the serial port for the synthesizer (0-based).");
+MODULE_PARM_DESC(dev, "Set the device e.g. ttyUSB0, for the synthesizer.");
MODULE_PARM_DESC(start, "Start the synthesizer once it is loaded.");
module_spk_synth(synth_apollo);
--- a/drivers/staging/speakup/speakup_audptr.c
+++ b/drivers/staging/speakup/speakup_audptr.c
@@ -100,6 +100,7 @@ static struct spk_synth synth_audptr = {
.trigger = 50,
.jiffies = 30,
.full = 18000,
+ .dev_name = SYNTH_DEFAULT_DEV,
.startup = SYNTH_START,
.checkval = SYNTH_CHECK,
.vars = vars,
@@ -162,9 +163,11 @@ static int synth_probe(struct spk_synth
}
module_param_named(ser, synth_audptr.ser, int, 0444);
+module_param_named(dev, synth_audptr.dev_name, charp, S_IRUGO);
module_param_named(start, synth_audptr.startup, short, 0444);
MODULE_PARM_DESC(ser, "Set the serial port for the synthesizer (0-based).");
+MODULE_PARM_DESC(dev, "Set the device e.g. ttyUSB0, for the synthesizer.");
MODULE_PARM_DESC(start, "Start the synthesizer once it is loaded.");
module_spk_synth(synth_audptr);
--- a/drivers/staging/speakup/speakup_bns.c
+++ b/drivers/staging/speakup/speakup_bns.c
@@ -93,6 +93,7 @@ static struct spk_synth synth_bns = {
.trigger = 50,
.jiffies = 50,
.full = 4,
+ .dev_name = SYNTH_DEFAULT_DEV,
.startup = SYNTH_START,
.checkval = SYNTH_CHECK,
.vars = vars,
@@ -119,9 +120,11 @@ static struct spk_synth synth_bns = {
};
module_param_named(ser, synth_bns.ser, int, 0444);
+module_param_named(dev, synth_bns.dev_name, charp, S_IRUGO);
module_param_named(start, synth_bns.startup, short, 0444);
MODULE_PARM_DESC(ser, "Set the serial port for the synthesizer (0-based).");
+MODULE_PARM_DESC(dev, "Set the device e.g. ttyUSB0, for the synthesizer.");
MODULE_PARM_DESC(start, "Start the synthesizer once it is loaded.");
module_spk_synth(synth_bns);
--- a/drivers/staging/speakup/speakup_decext.c
+++ b/drivers/staging/speakup/speakup_decext.c
@
[patch v2] tty: define tty_open_by_driver when CONFIG_TTY is not defined
This patch adds definition of tty_open_by_driver when CONFIG_TTY is not
defined. This was supposed to have been included in commit
12e84c71b7d4ee38d51377fd494ac748ee4e6912 ("tty: export
tty_open_by_driver"). The patch follows convention for other such
functions and returns NULL.
Signed-off-by: Okash Khawaja
---
include/linux/tty.h |3 +++
1 file changed, 3 insertions(+)
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -422,6 +422,9 @@ static inline int __init tty_init(void)
{ return 0; }
static inline const char *tty_name(const struct tty_struct *tty)
{ return "(none)"; }
+static inline struct tty_struct *tty_open_by_driver(dev_t device,
+ struct inode *inode, struct file *filp)
+{ return NULL; }
#endif
extern struct ktermios tty_std_termios;
Re: [patch 1/2] staging: speakup: add function to convert dev name to number
On Sat, Jun 17, 2017 at 12:25:34PM +0200, Greg Kroah-Hartman wrote: > On Sat, Jun 17, 2017 at 11:16:44AM +0100, Okash Khawaja wrote: > > Hi, > > > > On Thu, Jun 15, 2017 at 07:52:51AM +0100, Okash Khawaja wrote: > > > On Wed, Jun 14, 2017 at 03:04:18PM +0200, Greg Kroah-Hartman wrote: > > > > The console stuff is odd though, but that is each driver defining the > > > > name for itself, major/minor does not apply. Also, a separate driver is > > > > not having to figure this all out. I wonder if we could just add a tty > > > > core function for this as it does know the name of everything that has > > > > been registered with it, right? > > > > > > I can start working on a patch for this. I'm still new to the tty code > > > so will follow up with any questions I might have. > > > > I've put together this function for converting device name to number. > > It traverses tty_drivers list looking for corresponding dev name and > > index. Is this the right approach? > > Yes, this looks great, nice job! It is a lot simpler than your previous > function, and now you are not limited to just a subset off the different > serial ports in the system. > > Keep up the good work, it's really appreciated. Great, thanks. I'll re-send the full patch set as v2. Okash
Re: [patch 1/2] staging: speakup: add function to convert dev name to number
Hi,
On Thu, Jun 15, 2017 at 07:52:51AM +0100, Okash Khawaja wrote:
> On Wed, Jun 14, 2017 at 03:04:18PM +0200, Greg Kroah-Hartman wrote:
> > The console stuff is odd though, but that is each driver defining the
> > name for itself, major/minor does not apply. Also, a separate driver is
> > not having to figure this all out. I wonder if we could just add a tty
> > core function for this as it does know the name of everything that has
> > been registered with it, right?
>
> I can start working on a patch for this. I'm still new to the tty code
> so will follow up with any questions I might have.
I've put together this function for converting device name to number.
It traverses tty_drivers list looking for corresponding dev name and
index. Is this the right approach?
Thanks,
Okash
---
drivers/tty/tty_io.c | 45 +
include/linux/tty.h |4
2 files changed, 49 insertions(+)
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -325,6 +325,51 @@ static struct tty_driver *get_tty_driver
return NULL;
}
+/**
+ * tty_dev_name_to_number - return dev_t for device name
+ * @device_name: user space name of device under /dev
+ * @dev_no: pointer to dev_t that this function will populate
+ *
+ * This function converts device names like ttyS0 or ttyUSB1 into dev_t
+ * like (4, 64) or (188, 1). If no corresponding driver is registered then
+ * the function returns -ENODEV.
+ *
+ * Locking: this acquires tty_mutex
+ */
+int tty_dev_name_to_number(char *dev_name, dev_t *dev_no)
+{
+ struct tty_driver *p;
+ int rv, index, prefix_length = 0;
+
+ while (!isdigit(*(dev_name + prefix_length)) && prefix_length <
+ strlen(dev_name) )
+ prefix_length++;
+
+ if (prefix_length == strlen(dev_name))
+ return -EINVAL;
+
+ mutex_lock(&tty_mutex);
+
+ list_for_each_entry(p, &tty_drivers, tty_drivers)
+ if (prefix_length == strlen(p->name) && strncmp(dev_name,
+ p->name, prefix_length) == 0) {
+ rv = kstrtoint(dev_name + prefix_length, 10, &index);
+ if (rv) {
+ mutex_unlock(&tty_mutex);
+ return rv;
+ }
+ if (index < p->num) {
+ *dev_no = MKDEV(p->major, p->minor_start +
index);
+ mutex_unlock(&tty_mutex);
+ return 0;
+ }
+ }
+
+ mutex_unlock(&tty_mutex);
+ return -ENODEV;
+}
+EXPORT_SYMBOL_GPL(tty_dev_name_to_number);
+
#ifdef CONFIG_CONSOLE_POLL
/**
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -402,6 +402,7 @@ extern int __init tty_init(void);
extern const char *tty_name(const struct tty_struct *tty);
extern struct tty_struct *tty_open_by_driver(dev_t device, struct inode *inode,
struct file *filp);
+extern int tty_dev_name_to_number(char *dev_name, dev_t *dev_no);
#else
static inline void tty_kref_put(struct tty_struct *tty)
{ }
@@ -422,6 +423,9 @@ static inline int __init tty_init(void)
{ return 0; }
static inline const char *tty_name(const struct tty_struct *tty)
{ return "(none)"; }
+extern int tty_dev_name_to_number(char *dev_name, dev_t *dev_no)
+{ return -ENOTSUPP; }
+
#endif
extern struct ktermios tty_std_termios;
tty: define tty_open_by_driver when CONFIG_TTY is not defined
This patch adds definition of tty_open_by_driver when CONFIG_TTY is not
defined. This was supposed to have been included in commit
12e84c71b7d4ee38d51377fd494ac748ee4e6912 ("tty: export
tty_open_by_driver"). The patch follows convention for other such
functions and returns NULL.
Signed-off-by: Okash Khawaja
---
include/linux/tty.h |3 +++
1 file changed, 3 insertions(+)
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -422,6 +422,9 @@ static inline int __init tty_init(void)
{ return 0; }
static inline const char *tty_name(const struct tty_struct *tty)
{ return "(none)"; }
+static struct tty_struct *tty_open_by_driver(dev_t device, struct inode *inode,
+ struct file *filp)
+{ return NULL; }
#endif
extern struct ktermios tty_std_termios;
Re: [patch 1/2] staging: speakup: add function to convert dev name to number
Hi, On Wed, Jun 14, 2017 at 9:23 AM, Dan Carpenter wrote: [...] > > Could you call it "dev_name" instead? I normally expect "dev" to be a > device struct. Thanks for the feedback. Will keep these in mind for next version of the patch. Okash
Re: [patch 1/2] staging: speakup: add function to convert dev name to number
Hi, On Wed, Jun 14, 2017 at 03:04:18PM +0200, Greg Kroah-Hartman wrote: > The console stuff is odd though, but that is each driver defining the > name for itself, major/minor does not apply. Also, a separate driver is > not having to figure this all out. I wonder if we could just add a tty > core function for this as it does know the name of everything that has > been registered with it, right? I can start working on a patch for this. I'm still new to the tty code so will follow up with any questions I might have. Thanks, Okash
[patch 2/2] staging: speakup: make ttyio synths use device name
This patch introduces new module parameter, dev, which takes a string
representing the device that the external synth is connected to, e.g.
ttyS0, ttyUSB0 etc. This is then used to communicate with the synth.
That way, speakup can support more than ttyS*. As of this patch, it
only supports ttyS*, ttyUSB* and selected synths for lp*. dev parameter
is only available for tty-migrated synths.
Users will either use dev or ser as both serve same purpose. This patch
maintains backward compatility by allowing ser to be specified. When
both are specified, whichever is non-default, i.e. not ttyS0, is used.
If both are non-default then dev is used.
Signed-off-by: Okash Khawaja
Reviewed-by: Samuel Thibault
---
drivers/staging/speakup/speakup_acntsa.c |3 +++
drivers/staging/speakup/speakup_apollo.c |3 +++
drivers/staging/speakup/speakup_audptr.c |3 +++
drivers/staging/speakup/speakup_bns.c|3 +++
drivers/staging/speakup/speakup_decext.c |3 +++
drivers/staging/speakup/speakup_dectlk.c |3 +++
drivers/staging/speakup/speakup_dummy.c |3 +++
drivers/staging/speakup/speakup_ltlk.c |3 +++
drivers/staging/speakup/speakup_spkout.c |3 +++
drivers/staging/speakup/speakup_txprt.c |3 +++
drivers/staging/speakup/spk_ttyio.c | 15 +++
11 files changed, 37 insertions(+), 8 deletions(-)
--- a/drivers/staging/speakup/speakup_acntsa.c
+++ b/drivers/staging/speakup/speakup_acntsa.c
@@ -96,6 +96,7 @@ static struct spk_synth synth_acntsa = {
.trigger = 50,
.jiffies = 30,
.full = 4,
+ .dev = SYNTH_DEFAULT_DEV,
.startup = SYNTH_START,
.checkval = SYNTH_CHECK,
.vars = vars,
@@ -135,9 +136,11 @@ static int synth_probe(struct spk_synth
}
module_param_named(ser, synth_acntsa.ser, int, 0444);
+module_param_named(dev, synth_acntsa.dev, charp, S_IRUGO);
module_param_named(start, synth_acntsa.startup, short, 0444);
MODULE_PARM_DESC(ser, "Set the serial port for the synthesizer (0-based).");
+MODULE_PARM_DESC(dev, "Set the device e.g. ttyUSB0, for the synthesizer.");
MODULE_PARM_DESC(start, "Start the synthesizer once it is loaded.");
module_spk_synth(synth_acntsa);
--- a/drivers/staging/speakup/speakup_apollo.c
+++ b/drivers/staging/speakup/speakup_apollo.c
@@ -105,6 +105,7 @@ static struct spk_synth synth_apollo = {
.trigger = 50,
.jiffies = 50,
.full = 4,
+ .dev = SYNTH_DEFAULT_DEV,
.startup = SYNTH_START,
.checkval = SYNTH_CHECK,
.vars = vars,
@@ -199,9 +200,11 @@ static void do_catch_up(struct spk_synth
}
module_param_named(ser, synth_apollo.ser, int, 0444);
+module_param_named(dev, synth_apollo.dev, charp, S_IRUGO);
module_param_named(start, synth_apollo.startup, short, 0444);
MODULE_PARM_DESC(ser, "Set the serial port for the synthesizer (0-based).");
+MODULE_PARM_DESC(dev, "Set the device e.g. ttyUSB0, for the synthesizer.");
MODULE_PARM_DESC(start, "Start the synthesizer once it is loaded.");
module_spk_synth(synth_apollo);
--- a/drivers/staging/speakup/speakup_audptr.c
+++ b/drivers/staging/speakup/speakup_audptr.c
@@ -100,6 +100,7 @@ static struct spk_synth synth_audptr = {
.trigger = 50,
.jiffies = 30,
.full = 18000,
+ .dev = SYNTH_DEFAULT_DEV,
.startup = SYNTH_START,
.checkval = SYNTH_CHECK,
.vars = vars,
@@ -162,9 +163,11 @@ static int synth_probe(struct spk_synth
}
module_param_named(ser, synth_audptr.ser, int, 0444);
+module_param_named(dev, synth_audptr.dev, charp, S_IRUGO);
module_param_named(start, synth_audptr.startup, short, 0444);
MODULE_PARM_DESC(ser, "Set the serial port for the synthesizer (0-based).");
+MODULE_PARM_DESC(dev, "Set the device e.g. ttyUSB0, for the synthesizer.");
MODULE_PARM_DESC(start, "Start the synthesizer once it is loaded.");
module_spk_synth(synth_audptr);
--- a/drivers/staging/speakup/speakup_bns.c
+++ b/drivers/staging/speakup/speakup_bns.c
@@ -93,6 +93,7 @@ static struct spk_synth synth_bns = {
.trigger = 50,
.jiffies = 50,
.full = 4,
+ .dev = SYNTH_DEFAULT_DEV,
.startup = SYNTH_START,
.checkval = SYNTH_CHECK,
.vars = vars,
@@ -119,9 +120,11 @@ static struct spk_synth synth_bns = {
};
module_param_named(ser, synth_bns.ser, int, 0444);
+module_param_named(dev, synth_bns.dev, charp, S_IRUGO);
module_param_named(start, synth_bns.startup, short, 0444);
MODULE_PARM_DESC(ser, "Set the serial port for the synthesizer (0-based).");
+MODULE_PARM_DESC(dev, "Set the device e.g. ttyUSB0, for the synthesizer.");
MODULE_PARM_DESC(start, "Start the synthesizer once it is loaded.");
module_spk_synth(synth_bns);
--- a/drivers/staging/speakup/speakup_decext.c
+++ b/drivers/staging/speakup/speakup_decext.c
@@ -120,6 +120,7 @@ static struct spk_synth
[patch 1/2] staging: speakup: add function to convert dev name to number
The function converts strings like ttyS0 and ttyUSB0 to dev_t like
(4, 64) and (188, 0). Subsequent patch in this set will call it to
convert user-supplied device into device number. The function does
some basic sanity checks on the string passed in. It currently supports
ttyS*, ttyUSB* and, for selected synths, lp*.
In order to do this, the patch also introduces a string member variable
named 'dev' to struct spk_synth. 'dev' represents the device name -
ttyUSB0 etc - which needs conversion to dev_t.
Signed-off-by: Okash Khawaja
Reviewed-by: Samuel Thibault
---
drivers/staging/speakup/spk_priv.h |2
drivers/staging/speakup/spk_ttyio.c | 105
drivers/staging/speakup/spk_types.h |1
3 files changed, 108 insertions(+)
--- a/drivers/staging/speakup/spk_priv.h
+++ b/drivers/staging/speakup/spk_priv.h
@@ -40,6 +40,8 @@
#define KT_SPKUP 15
#define SPK_SYNTH_TIMEOUT 10 /* in micro-seconds */
+#define SYNTH_DEFAULT_DEV "ttyS0"
+#define SYNTH_DEFAULT_SER 0
const struct old_serial_port *spk_serial_init(int index);
void spk_stop_serial_interrupt(void);
--- a/drivers/staging/speakup/spk_ttyio.c
+++ b/drivers/staging/speakup/spk_ttyio.c
@@ -7,6 +7,13 @@
#include "spk_types.h"
#include "spk_priv.h"
+/* Supported device types */
+#define DEV_PREFIX_TTYS "ttyS"
+#define DEV_PREFIX_TTYUSB "ttyUSB"
+#define DEV_PREFIX_LP "lp"
+
+const char *lp_supported[] = { "acntsa", "bns", "dummy", "txprt" };
+
struct spk_ldisc_data {
char buf;
struct semaphore sem;
@@ -16,6 +23,104 @@ struct spk_ldisc_data {
static struct spk_synth *spk_ttyio_synth;
static struct tty_struct *speakup_tty;
+static int name_to_dev(const char *name, dev_t *dev_no)
+{
+int maj = -1, min = -1;
+
+if (strncmp(name, DEV_PREFIX_TTYS, strlen(DEV_PREFIX_TTYS)) == 0) {
+if (kstrtoint(name + strlen(DEV_PREFIX_TTYS), 10, &min)) {
+ pr_err("speakup: Invalid ser param. Must be \
+ between 0 and 191 inclusive.\n");
+return -EINVAL;
+}
+maj = 4;
+
+if (min < 0 || min > 191) {
+ pr_err("speakup: Invalid ser param. Must be \
+ between 0 and 191 inclusive.\n");
+return -EINVAL;
+}
+min = min + 64;
+} else if (strncmp(name, DEV_PREFIX_TTYUSB, strlen(DEV_PREFIX_TTYUSB))
+ == 0) {
+if (kstrtoint(name + strlen(DEV_PREFIX_TTYUSB), 10, &min)) {
+pr_err("speakup: Invalid ttyUSB number. \
+ Must be a number from 0 onwards\n");
+return -EINVAL;
+}
+maj = 188;
+
+if (min < 0) {
+pr_err("speakup: Invalid ttyUSB number. \
+ Must be a number from 0 onwards\n");
+return -EINVAL;
+}
+} else if (strncmp(name, DEV_PREFIX_LP, strlen(DEV_PREFIX_LP)) == 0) {
+if (kstrtoint(name + strlen(DEV_PREFIX_LP), 10, &min)) {
+pr_warn("speakup: Invalid lp number. \
+ Must be a number from 0 onwards\n");
+return -EINVAL;
+}
+maj = 6;
+
+if (min < 0) {
+pr_warn("speakup: Invalid lp number. \
+ Must be a number from 0 onwards\n");
+return -EINVAL;
+}
+}
+
+if (maj == -1 || min == -1)
+return -EINVAL;
+
+/* if here, maj and min must be valid */
+*dev_no = MKDEV(maj, min);
+
+return 0;
+}
+
+int ser_to_dev(int ser, dev_t *dev_no)
+{
+ if (ser < 0 || ser > (255 - 64)) {
+pr_err("speakup: Invalid ser param. \
+ Must be between 0 and 191 inclusive.\n");
+
+ return -EINVAL;
+}
+
+ *dev_no = MKDEV(4, (64 + ser));
+ return 0;
+}
+
+static int get_dev_to_use(struct spk_synth *synth, dev_t *dev_no)
+{
+ /* use ser only when dev is not specified */
+ if (strcmp(synth->dev, SYNTH_DEFAULT_DEV) || synth->ser ==
SYNTH_DEFAULT_SER) {
+ /* for /dev/lp* check if synth is supported */
+ if (strncmp(synth->dev, DEV_PREFIX_LP, strlen(DEV_PREFIX_LP))
== 0) {
+ int i;
+
+ for (i = 0; i < ARRAY_SIZE(lp_supported); i++) {
+ if (strcmp(synth->nam
[patch 0/2] staging: speakup: support more than ttyS*
Hi, These patches extend speakup support to ttyUSB* and lp*. They introduce a new module param dev whose function is similar to ser but instead of taking serial port number as argument, it takes strings like ttyS0 or ttyUSB0. First patch just adds functionality to convert such strings into dev_t. Second patch makes use of that functionlity. Thanks, Okash
[patch] staging: speakup: migrate bns to tty
Migration of bns was missed out in the patch
https://patchwork.kernel.org/patch/9727725/. This patch does it by
updating relevant function pointers, just like in the patch linked
above.
Signed-off-by: Okash Khawaja
Reviewed-by: Samuel Thibault
---
drivers/staging/speakup/speakup_bns.c |8
1 file changed, 4 insertions(+), 4 deletions(-)
--- a/drivers/staging/speakup/speakup_bns.c
+++ b/drivers/staging/speakup/speakup_bns.c
@@ -96,10 +96,10 @@ static struct spk_synth synth_bns = {
.startup = SYNTH_START,
.checkval = SYNTH_CHECK,
.vars = vars,
- .io_ops = &spk_serial_io_ops,
- .probe = spk_serial_synth_probe,
- .release = spk_serial_release,
- .synth_immediate = spk_serial_synth_immediate,
+ .io_ops = &spk_ttyio_ops,
+ .probe = spk_ttyio_synth_probe,
+ .release = spk_ttyio_release,
+ .synth_immediate = spk_ttyio_synth_immediate,
.catch_up = spk_do_catch_up,
.flush = spk_synth_flush,
.is_alive = spk_synth_is_alive_restart,
[patch] staging: speakup: remove unused code
In spk_ttyio_release we read tty's index but never do anything with it.
The patch removes this dead code.
Signed-off-by: Okash Khawaja
Reviewed-by: Samuel Thibault
---
drivers/staging/speakup/spk_ttyio.c |3 ---
1 file changed, 3 deletions(-)
--- a/drivers/staging/speakup/spk_ttyio.c
+++ b/drivers/staging/speakup/spk_ttyio.c
@@ -247,13 +247,10 @@ EXPORT_SYMBOL_GPL(spk_ttyio_synth_probe)
void spk_ttyio_release(void)
{
- int idx;
-
if (!speakup_tty)
return;
tty_lock(speakup_tty);
- idx = speakup_tty->index;
if (speakup_tty->ops->close)
speakup_tty->ops->close(speakup_tty, NULL);
[patch] staging: speakup: check for null before calling TTY's flush_buffer
We should check the flush_buffer method of a tty for null before
invoking it. Some drivers such as usbserial don't implement
flush_buffer. This will be required for upcoming patches where we expand
spk_ttyio to support more than just ttyS*.
Signed-off-by: Okash Khawaja
Reviewed-by: Samuel Thibault
---
drivers/staging/speakup/spk_ttyio.c |3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
--- a/drivers/staging/speakup/spk_ttyio.c
+++ b/drivers/staging/speakup/spk_ttyio.c
@@ -227,7 +227,8 @@ static unsigned char spk_ttyio_in_nowait
static void spk_ttyio_flush_buffer(void)
{
- speakup_tty->ops->flush_buffer(speakup_tty);
+ if (speakup_tty->ops->flush_buffer)
+ speakup_tty->ops->flush_buffer(speakup_tty);
}
int spk_ttyio_synth_probe(struct spk_synth *synth)
Re: [patch v2 6/6] staging: speakup: flush tty buffers and ensure hardware flow control
> On 16 May 2017, at 13:28, Greg Kroah-Hartman > wrote: > >> On Tue, May 16, 2017 at 01:19:10PM +0100, Okash Khawaja wrote: >> Hi, >> >> On Tue, May 16, 2017 at 1:00 PM, Greg Kroah-Hartman >> wrote: >>> On Mon, May 15, 2017 at 06:45:37PM +0100, Okash Khawaja wrote: >>>> This patch fixes the issue where TTY-migrated synths would take a while to >>>> shut up after hitting numpad enter key. When calling synth_flush, even >>>> though XOFF character is sent as high priority, data buffered in TTY layer >>>> is still sent to the synth. This patch flushes that buffered data when >>>> synth_flush is called. >>> >>> Minor nit, please line-wrap your changelog text at 72 columns so that I >>> don't have to do it "by hand". >> Sure, will do. >> >>> >>>> >>>> It also tries to ensure that hardware flow control is enabled, by setting >>>> CRTSCTS using tty's termios. >>>> >>>> Reported-by: John Covici >>>> Signed-off-by: Okash Khawaja >>>> Reviewed-by: Samuel Thibault >>>> >>>> Index: linux-staging/drivers/staging/speakup/serialio.c >>>> === >>> >>> Are you using git? These lines are odd... >> They come from quilt. Haven't checked yet if there is an option to >> turn them off. > > There is, here's what is in my .quiltrc: > > QUILT_REFRESH_ARGS="--diffstat --strip-trailing-whitespace --no-timestamps > --no-index --sort -p1 -p ab" > QUILT_DIFF_ARGS="--no-timestamps --no-index --sort --color=auto -p ab" > QUILT_DIFF_OPTS="-p" > > That will give you the diffstat in the patch as well, which is always > helpful to reviewers. Thanks very much. Will update mine.
Re: [patch v2 6/6] staging: speakup: flush tty buffers and ensure hardware flow control
Hi, On Tue, May 16, 2017 at 1:00 PM, Greg Kroah-Hartman wrote: > On Mon, May 15, 2017 at 06:45:37PM +0100, Okash Khawaja wrote: >> This patch fixes the issue where TTY-migrated synths would take a while to >> shut up after hitting numpad enter key. When calling synth_flush, even >> though XOFF character is sent as high priority, data buffered in TTY layer >> is still sent to the synth. This patch flushes that buffered data when >> synth_flush is called. > > Minor nit, please line-wrap your changelog text at 72 columns so that I > don't have to do it "by hand". Sure, will do. > >> >> It also tries to ensure that hardware flow control is enabled, by setting >> CRTSCTS using tty's termios. >> >> Reported-by: John Covici >> Signed-off-by: Okash Khawaja >> Reviewed-by: Samuel Thibault >> >> Index: linux-staging/drivers/staging/speakup/serialio.c >> === > > Are you using git? These lines are odd... They come from quilt. Haven't checked yet if there is an option to turn them off. Thanks, Okash
[patch v2 1/6] tty: export tty_open_by_driver
This exports tty_open_by_driver so that it can be called from other places
inside the kernel. The checks for null file pointer are based on Alan Cox's
patch here:
http://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1215095.html.
Description below is quoted from it:
"[RFC] tty_port: allow a port to be opened with a tty that has no file handle
Let us create tty objects entirely in kernel space. Untested proposal to
show why all the ideas around rewriting half the uart stack are not needed.
With this a kernel created non file backed tty object could be used to
handle
data, and set terminal modes. Not all ldiscs can cope with this as N_TTY in
particular has to work back to the fs/tty layer.
The tty_port code is however otherwise clean of file handles as far as I can
tell as is the low level tty port write path used by the ldisc, the
configuration low level interfaces and most of the ldiscs.
Currently you don't have any exposure to see tty hangups because those are
built around the file layer. However a) it's a fixed port so you probably
don't care about that b) if you do we can add a callback and c) you almost
certainly don't want the userspace tear down/rebuild behaviour anyway.
This should however be sufficient if we wanted for example to enumerate all
the bluetooth bound fixed ports via ACPI and make them directly available.
It doesn't deal with the case of a user opening a port that's also kernel
opened and that would need some locking out (so it returned EBUSY if bound
to a kernel device of some kind). That needs resolving along with how you
"up" or "down" your new bluetooth device, or enumerate it while providing
the existing tty API to avoid regressions (and to debug)."
The exported funtion is used later in this patch set to gain access to
tty_struct.
Signed-off-by: Okash Khawaja
Reviewed-by: Samuel Thibault
Index: linux-staging/drivers/tty/tty_io.c
===
--- linux-staging.orig/drivers/tty/tty_io.c
+++ linux-staging/drivers/tty/tty_io.c
@@ -1369,7 +1369,10 @@ static struct tty_struct *tty_driver_loo
struct tty_struct *tty;
if (driver->ops->lookup)
- tty = driver->ops->lookup(driver, file, idx);
+ if (!file)
+ tty = ERR_PTR(-EIO);
+ else
+ tty = driver->ops->lookup(driver, file, idx);
else
tty = driver->ttys[idx];
@@ -2001,7 +2004,7 @@ static struct tty_driver *tty_lookup_dri
struct tty_driver *console_driver = console_device(index);
if (console_driver) {
driver = tty_driver_kref_get(console_driver);
- if (driver) {
+ if (driver && filp) {
/* Don't let /dev/console block */
filp->f_flags |= O_NONBLOCK;
break;
@@ -2034,7 +2037,7 @@ static struct tty_driver *tty_lookup_dri
* - concurrent tty driver removal w/ lookup
* - concurrent tty removal from driver table
*/
-static struct tty_struct *tty_open_by_driver(dev_t device, struct inode *inode,
+struct tty_struct *tty_open_by_driver(dev_t device, struct inode *inode,
struct file *filp)
{
struct tty_struct *tty;
@@ -2079,6 +2082,7 @@ out:
tty_driver_kref_put(driver);
return tty;
}
+EXPORT_SYMBOL(tty_open_by_driver);
/**
* tty_open- open a tty device
Index: linux-staging/include/linux/tty.h
===
--- linux-staging.orig/include/linux/tty.h
+++ linux-staging/include/linux/tty.h
@@ -401,6 +401,8 @@ extern struct tty_struct *get_current_tt
/* tty_io.c */
extern int __init tty_init(void);
extern const char *tty_name(const struct tty_struct *tty);
+extern struct tty_struct *tty_open_by_driver(dev_t device, struct inode *inode,
+ struct file *filp);
#else
static inline void console_init(void)
{ }
[patch v2 6/6] staging: speakup: flush tty buffers and ensure hardware flow control
This patch fixes the issue where TTY-migrated synths would take a while to shut
up after hitting numpad enter key. When calling synth_flush, even though XOFF
character is sent as high priority, data buffered in TTY layer is still sent to
the synth. This patch flushes that buffered data when synth_flush is called.
It also tries to ensure that hardware flow control is enabled, by setting
CRTSCTS using tty's termios.
Reported-by: John Covici
Signed-off-by: Okash Khawaja
Reviewed-by: Samuel Thibault
Index: linux-staging/drivers/staging/speakup/serialio.c
===
--- linux-staging.orig/drivers/staging/speakup/serialio.c
+++ linux-staging/drivers/staging/speakup/serialio.c
@@ -30,6 +30,7 @@
static void spk_serial_tiocmset(unsigned int set, unsigned int clear);
static unsigned char spk_serial_in(void);
static unsigned char spk_serial_in_nowait(void);
+static void spk_serial_flush_buffer(void);
struct spk_io_ops spk_serial_io_ops = {
.synth_out = spk_serial_out,
@@ -37,6 +38,7 @@
.tiocmset = spk_serial_tiocmset,
.synth_in = spk_serial_in,
.synth_in_nowait = spk_serial_in_nowait,
+ .flush_buffer = spk_serial_flush_buffer,
};
EXPORT_SYMBOL_GPL(spk_serial_io_ops);
@@ -268,6 +270,11 @@
return inb_p(speakup_info.port_tts + UART_RX);
}
+static void spk_serial_flush_buffer(void)
+{
+ /* TODO: flush the UART 16550 buffer */
+}
+
static int spk_serial_out(struct spk_synth *in_synth, const char ch)
{
if (in_synth->alive && spk_wait_for_xmitr(in_synth)) {
Index: linux-staging/drivers/staging/speakup/spk_ttyio.c
===
--- linux-staging.orig/drivers/staging/speakup/spk_ttyio.c
+++ linux-staging/drivers/staging/speakup/spk_ttyio.c
@@ -85,6 +85,7 @@
static void spk_ttyio_tiocmset(unsigned int set, unsigned int clear);
static unsigned char spk_ttyio_in(void);
static unsigned char spk_ttyio_in_nowait(void);
+static void spk_ttyio_flush_buffer(void);
struct spk_io_ops spk_ttyio_ops = {
.synth_out = spk_ttyio_out,
@@ -92,13 +93,22 @@
.tiocmset = spk_ttyio_tiocmset,
.synth_in = spk_ttyio_in,
.synth_in_nowait = spk_ttyio_in_nowait,
+ .flush_buffer = spk_ttyio_flush_buffer,
};
EXPORT_SYMBOL_GPL(spk_ttyio_ops);
+static inline void get_termios(struct tty_struct *tty, struct ktermios
*out_termios)
+{
+ down_read(&tty->termios_rwsem);
+ *out_termios = tty->termios;
+ up_read(&tty->termios_rwsem);
+}
+
static int spk_ttyio_initialise_ldisc(int ser)
{
int ret = 0;
struct tty_struct *tty;
+ struct ktermios tmp_termios;
ret = tty_register_ldisc(N_SPEAKUP, &spk_ttyio_ldisc_ops);
if (ret) {
@@ -127,6 +137,20 @@
}
clear_bit(TTY_HUPPED, &tty->flags);
+ /* ensure hardware flow control is enabled */
+ get_termios(tty, &tmp_termios);
+ if (!(tmp_termios.c_cflag & CRTSCTS)) {
+ tmp_termios.c_cflag |= CRTSCTS;
+ tty_set_termios(tty, &tmp_termios);
+ /*
+* check c_cflag to see if it's updated as tty_set_termios may
not return
+* error even when no tty bits are changed by the request.
+*/
+ get_termios(tty, &tmp_termios);
+ if (!(tmp_termios.c_cflag & CRTSCTS))
+ pr_warn("speakup: Failed to set hardware flow
control\n");
+ }
+
tty_unlock(tty);
ret = tty_set_ldisc(tty, N_SPEAKUP);
@@ -201,6 +225,11 @@
return (rv == 0xff) ? 0 : rv;
}
+static void spk_ttyio_flush_buffer(void)
+{
+ speakup_tty->ops->flush_buffer(speakup_tty);
+}
+
int spk_ttyio_synth_probe(struct spk_synth *synth)
{
int rv = spk_ttyio_initialise_ldisc(synth->ser);
Index: linux-staging/drivers/staging/speakup/spk_types.h
===
--- linux-staging.orig/drivers/staging/speakup/spk_types.h
+++ linux-staging/drivers/staging/speakup/spk_types.h
@@ -154,6 +154,7 @@
void (*tiocmset)(unsigned int set, unsigned int clear);
unsigned char (*synth_in)(void);
unsigned char (*synth_in_nowait)(void);
+ void (*flush_buffer)(void);
};
struct spk_synth {
Index: linux-staging/drivers/staging/speakup/speakup_audptr.c
===
--- linux-staging.orig/drivers/staging/speakup/speakup_audptr.c
+++ linux-staging/drivers/staging/speakup/speakup_audptr.c
@@ -127,6 +127,7 @@
static void synth_flush(struct spk_synth *synth)
{
+ synth->io_ops->flush_buffer();
synth->io_ops->send_xchar(SYNTH_CLEAR);
synth->io_ops->synth_out(synth, PROCSPEECH);
}
Index: linux-staging/drivers/staging/speakup/speakup_decext.c
[patch v2 4/6] staging: speakup: add send_xchar, tiocmset and input functionality for tty
This patch adds further TTY-based functionality, specifically implementation
of send_xchar and tiocmset methods, and input. send_xchar and tiocmset
methods simply delegate to corresponding TTY operations.
For input, it implements the receive_buf2 callback in tty_ldisc_ops of
speakup's ldisc. If a synth defines read_buff_add method then receive_buf2
simply delegates to that and returns.
For spk_ttyio_in, the data is passed from receive_buf2 thread to
spk_ttyio_in thread through spk_ldisc_data structure. It has following
members:
- char buf: represents data received
- struct semaphore sem: used to signal to spk_ttyio_in thread that data
is available to be read without having to busy wait
- bool buf_free: this is used in comination with mb() calls to syncronise
the two threads over buf
receive_buf2 only writes to buf if buf_free is true. The check for buf_free
and writing to buf are separated by mb() to ensure that spk_ttyio_in has read
buf before receive_buf2 writes to it. After writing, it ups the semaphore to
signal to spk_ttyio_in that there is now data to read.
spk_ttyio_in waits for data to read by downing the semaphore. Thus when
signalled by receive_buf2 thread above, it reads from buf and sets buf_free
to true. These two operations are separated by mb() to ensure that
receive_buf2 thread finds buf_free to be true only after buf has been read.
After that spk_ttyio_in calls tty_schedule_flip for subsequent data to come
in through receive_buf2.
Signed-off-by: Okash Khawaja
Reviewed-by: Samuel Thibault
Index: linux-staging/drivers/staging/speakup/spk_ttyio.c
===
--- linux-staging.orig/drivers/staging/speakup/spk_ttyio.c
+++ linux-staging/drivers/staging/speakup/spk_ttyio.c
@@ -1,36 +1,97 @@
#include
#include
+#include
+#include
#include "speakup.h"
#include "spk_types.h"
+#include "spk_priv.h"
+struct spk_ldisc_data {
+ char buf;
+ struct semaphore sem;
+ bool buf_free;
+};
+
+static struct spk_synth *spk_ttyio_synth;
static struct tty_struct *speakup_tty;
static int spk_ttyio_ldisc_open(struct tty_struct *tty)
{
+ struct spk_ldisc_data *ldisc_data;
+
if (tty->ops->write == NULL)
return -EOPNOTSUPP;
speakup_tty = tty;
+ ldisc_data = kmalloc(sizeof(struct spk_ldisc_data), GFP_KERNEL);
+ if (!ldisc_data) {
+ pr_err("speakup: Failed to allocate ldisc_data.\n");
+ return -ENOMEM;
+ }
+
+ sema_init(&ldisc_data->sem, 0);
+ ldisc_data->buf_free = true;
+ speakup_tty->disc_data = ldisc_data;
+
return 0;
}
static void spk_ttyio_ldisc_close(struct tty_struct *tty)
{
+ kfree(speakup_tty->disc_data);
speakup_tty = NULL;
}
+static int spk_ttyio_receive_buf2(struct tty_struct *tty,
+ const unsigned char *cp, char *fp, int count)
+{
+ struct spk_ldisc_data *ldisc_data = tty->disc_data;
+
+ if (spk_ttyio_synth->read_buff_add) {
+ int i;
+ for (i = 0; i < count; i++)
+ spk_ttyio_synth->read_buff_add(cp[i]);
+
+ return count;
+ }
+
+ if (!ldisc_data->buf_free)
+ /* ttyio_in will tty_schedule_flip */
+ return 0;
+
+ /* Make sure the consumer has read buf before we have seen
+ * buf_free == true and overwrite buf */
+ mb();
+
+ ldisc_data->buf = cp[0];
+ ldisc_data->buf_free = false;
+ up(&ldisc_data->sem);
+
+ return 1;
+}
+
static struct tty_ldisc_ops spk_ttyio_ldisc_ops = {
.owner = THIS_MODULE,
.magic = TTY_LDISC_MAGIC,
.name = "speakup_ldisc",
.open = spk_ttyio_ldisc_open,
.close = spk_ttyio_ldisc_close,
+ .receive_buf2 = spk_ttyio_receive_buf2,
};
static int spk_ttyio_out(struct spk_synth *in_synth, const char ch);
+static void spk_ttyio_send_xchar(char ch);
+static void spk_ttyio_tiocmset(unsigned int set, unsigned int clear);
+static unsigned char spk_ttyio_in(void);
+static unsigned char spk_ttyio_in_nowait(void);
+
struct spk_io_ops spk_ttyio_ops = {
.synth_out = spk_ttyio_out,
+ .send_xchar = spk_ttyio_send_xchar,
+ .tiocmset = spk_ttyio_tiocmset,
+ .synth_in = spk_ttyio_in,
+ .synth_in_nowait = spk_ttyio_in_nowait,
};
EXPORT_SYMBOL_GPL(spk_ttyio_ops);
@@ -95,6 +156,51 @@
return 0;
}
+static void spk_ttyio_send_xchar(char ch)
+{
+ speakup_tty->ops->send_xchar(speakup_tty, ch);
+}
+
+static void spk_ttyio_tiocmset(unsigned int set, unsigned int clear)
+{
+ speakup_tty->ops->tiocmset(speakup_tty, set, clear);
+}
+
+static unsigned char ttyio_in(int timeout)
+{
+ struct spk_ldisc_data *ldisc_data = speaku
[patch v2 5/6] staging: speakup: migrate apollo, ltlk, audptr, decext, dectlk and spkout
This patch simply uses the changes introduced in previous patches and migrates
apollo, ltlk, audptr, decext, spkout and dectlk. Migrations are straightforward
function pointer updates.
Signed-off by: Okash Khawaja
Reviewed-by: Samuel Thibault
Index: linux-staging/drivers/staging/speakup/speakup_apollo.c
===
--- linux-staging.orig/drivers/staging/speakup/speakup_apollo.c
+++ linux-staging/drivers/staging/speakup/speakup_apollo.c
@@ -22,9 +22,9 @@
#include
#include
#include
+#include /* for UART_MCR* constants */
#include "spk_priv.h"
-#include "serialio.h"
#include "speakup.h"
#define DRV_VERSION "2.21"
@@ -108,10 +108,10 @@
.startup = SYNTH_START,
.checkval = SYNTH_CHECK,
.vars = vars,
- .io_ops = &spk_serial_io_ops,
- .probe = spk_serial_synth_probe,
- .release = spk_serial_release,
- .synth_immediate = spk_serial_synth_immediate,
+ .io_ops = &spk_ttyio_ops,
+ .probe = spk_ttyio_synth_probe,
+ .release = spk_ttyio_release,
+ .synth_immediate = spk_ttyio_synth_immediate,
.catch_up = do_catch_up,
.flush = spk_synth_flush,
.is_alive = spk_synth_is_alive_restart,
Index: linux-staging/drivers/staging/speakup/speakup_ltlk.c
===
--- linux-staging.orig/drivers/staging/speakup/speakup_ltlk.c
+++ linux-staging/drivers/staging/speakup/speakup_ltlk.c
@@ -20,7 +20,6 @@
*/
#include "speakup.h"
#include "spk_priv.h"
-#include "serialio.h"
#include "speakup_dtlk.h" /* local header file for LiteTalk values */
#define DRV_VERSION "2.11"
@@ -111,10 +110,10 @@
.startup = SYNTH_START,
.checkval = SYNTH_CHECK,
.vars = vars,
- .io_ops = &spk_serial_io_ops,
+ .io_ops = &spk_ttyio_ops,
.probe = synth_probe,
- .release = spk_serial_release,
- .synth_immediate = spk_serial_synth_immediate,
+ .release = spk_ttyio_release,
+ .synth_immediate = spk_ttyio_synth_immediate,
.catch_up = spk_do_catch_up,
.flush = spk_synth_flush,
.is_alive = spk_synth_is_alive_restart,
@@ -159,7 +158,7 @@
{
int failed = 0;
- failed = spk_serial_synth_probe(synth);
+ failed = spk_ttyio_synth_probe(synth);
if (failed == 0)
synth_interrogate(synth);
synth->alive = !failed;
Index: linux-staging/drivers/staging/speakup/speakup_audptr.c
===
--- linux-staging.orig/drivers/staging/speakup/speakup_audptr.c
+++ linux-staging/drivers/staging/speakup/speakup_audptr.c
@@ -20,7 +20,6 @@
*/
#include "spk_priv.h"
#include "speakup.h"
-#include "serialio.h"
#define DRV_VERSION "2.11"
#define SYNTH_CLEAR 0x18 /* flush synth buffer */
@@ -104,10 +103,10 @@
.startup = SYNTH_START,
.checkval = SYNTH_CHECK,
.vars = vars,
- .io_ops = &spk_serial_io_ops,
+ .io_ops = &spk_ttyio_ops,
.probe = synth_probe,
- .release = spk_serial_release,
- .synth_immediate = spk_serial_synth_immediate,
+ .release = spk_ttyio_release,
+ .synth_immediate = spk_ttyio_synth_immediate,
.catch_up = spk_do_catch_up,
.flush = synth_flush,
.is_alive = spk_synth_is_alive_restart,
@@ -154,7 +153,7 @@
{
int failed;
- failed = spk_serial_synth_probe(synth);
+ failed = spk_ttyio_synth_probe(synth);
if (failed == 0)
synth_version(synth);
synth->alive = !failed;
Index: linux-staging/drivers/staging/speakup/speakup_decext.c
===
--- linux-staging.orig/drivers/staging/speakup/speakup_decext.c
+++ linux-staging/drivers/staging/speakup/speakup_decext.c
@@ -24,12 +24,12 @@
#include
#include "spk_priv.h"
-#include "serialio.h"
#include "speakup.h"
#define DRV_VERSION "2.14"
#define SYNTH_CLEAR 0x03
#define PROCSPEECH 0x0b
+
static volatile unsigned char last_char;
static void read_buff_add(u_char ch)
@@ -123,10 +123,10 @@
.startup = SYNTH_START,
.checkval = SYNTH_CHECK,
.vars = vars,
- .io_ops = &spk_serial_io_ops,
- .probe = spk_serial_synth_probe,
- .release = spk_serial_release,
- .synth_immediate = spk_serial_synth_immediate,
+ .io_ops = &spk_ttyio_ops,
+ .probe = spk_ttyio_synth_probe,
+ .release = spk_ttyio_release,
+ .synth_immediate = spk_ttyio_synth_immediate,
.catch_up = do_catch_up,
.flush = synth_flush,
.is_alive = spk_synth_is_alive_restart,
Index: linux-staging/drivers/staging/speakup/speakup_spkout.c
===
