[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 + punc_level. Also
[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 if punc_level
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 or don't echo keys.
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 being reviewed. Zero just speaks the l
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 staging both, for settling > > &g
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(_notifier_block);
unregister_vt_notifier(_notifier_block);
speakup_unregister_devsynth();
+ speakup_cancel_selection();
speakup_cancel_paste();
del_timer_sync(_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(>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 */
- if (new_sel_end > new_sel_start &&
- !atedge(new_sel_end,
[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(, sel, sizeof(*sel)))
+ return -EFAULT;
+
+ return set_selection_kernel(, 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(, 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(set_selection_kernel);
/* Insert the contents of the selection buffer into the
[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(, sel, sizeof(*sel)))
+ return -EFAULT;
+
+ return do_set_selection(, 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(, 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(_notifier_block);
unregister_vt_notifier(_notifier_block);
speakup_unregister_devsynth();
+ speakup_cancel_selection();
speakup_cancel_paste();
del_timer_sync(_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(>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 */
- if (new_sel_end > new_sel_start &&
- !atedge(new_sel_end,
[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 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 *)) == 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(_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(_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 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 *)) == 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(_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(_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 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 *)) == 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 = bit_offset + BTF_INT_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, _info, );
+
+ 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, _info, );
+ }
+
+ 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)
+ return NULL;
+ jsonw_pretty(jw, true);
[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 *)) == 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 = bit_offset + BTF_INT_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, _info, );
+
+ 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, _info, );
+ }
+
+ 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)
+ return NULL;
+ jsonw_pretty(jw, true);
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, _info, );
> > + } 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 ar
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, _info, );
> > + } 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 ar
[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, _info, );
+ } 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();
+
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(, key, value);
- else
+ if (json_output) {
+ if (btf)
+ do_dump_btf(btf, , key, value);
+ else
+ print_entry_json(, key, value);
+ } else
print_entry_plain(, key, value);
} else {
if (json_output) {
@@ -584,6 +671,7 @@ exit_free:
free(key);
free(value);
close(fd);
+ btf__free(btf);
return err;
}
[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, _info, );
+ } 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();
+
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(, key, value);
- else
+ if (json_output) {
+ if (btf)
+ do_dump_btf(btf, , key, value);
+ else
+ print_entry_json(, key, value);
+ } else
print_entry_plain(, 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] (&(>event_lock)->rlock){-.-...}, at: []
input_event+0x3e/0x80
[ 1293.803671]
[ 1293.803671] but task is already holding lock:
[ 1293.803740] (&(>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(&(>event_lock)->rlock);
[ 1293.804109] lock(&(>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: (>lock){-.-...}, at: []
serio_interrupt+0x2a/0x90 [serio]
[ 1293.804469] #1: (&(>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 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] (&(>event_lock)->rlock){-.-...}, at: []
input_event+0x3e/0x80
[ 1293.803671]
[ 1293.803671] but task is already holding lock:
[ 1293.803740] (&(>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(&(>event_lock)->rlock);
[ 1293.804109] lock(&(>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: (>lock){-.-...}, at: []
serio_interrupt+0x2a/0x90 [serio]
[ 1293.804469] #1: (&(>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 <okash.khaw...@gmail.com>
Reviewed-by: Samuel Thibault <samuel.thiba...@ens-lyon.org>
---
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(_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 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(_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 <okash.khaw...@gmail.com>
Reviewed-by: Samuel Thibault <samuel.thiba...@ens-lyon.org>
---
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(_tty_mutex);
kfree(speakup_tty->disc_data);
speakup_tty = NULL;
+ mutex_unlock(_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(_tty_mutex);
if (in_synth->alive && speakup_tty && speakup_tty->ops->write) {
int ret = speakup_tty->ops->write(speakup_tty, , 1);
+ mutex_unlock(_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(_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(_tty_mutex);
+ if (check_tty(speakup_tty)) {
+ mutex_unlock(_tty_mutex);
+ return;
+ }
+
speakup_tty->ops->send_xchar(speakup_tty, ch);
+ mutex_unlock(_tty_mutex);
}
static void spk_ttyio_tiocmset(unsigned int set, unsigned int clear)
{
+ mutex_lock(_tty_mutex);
+ if (check_tty(speakup_tty)) {
+ mutex_unlock(_tty_mutex);
+ return;
+ }
+
speakup_tty->ops->tiocmset(speakup_tty, set, clear);
+ mutex_unlock(_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(_tty_mutex);
+ if (check_tty(speakup_tty)) {
+ mutex_unlock(_tty_mutex);
+ return;
+ }
+
if (speakup_tty->ops->flush_buffer)
speakup_tty->ops->flush_buffer(speakup_tty);
+
+ mutex_unlock(_tty_mutex);
}
int spk_ttyio_synth_probe(struct spk_synth *synth)
[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(_tty_mutex);
kfree(speakup_tty->disc_data);
speakup_tty = NULL;
+ mutex_unlock(_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(_tty_mutex);
if (in_synth->alive && speakup_tty && speakup_tty->ops->write) {
int ret = speakup_tty->ops->write(speakup_tty, , 1);
+ mutex_unlock(_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(_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(_tty_mutex);
+ if (check_tty(speakup_tty)) {
+ mutex_unlock(_tty_mutex);
+ return;
+ }
+
speakup_tty->ops->send_xchar(speakup_tty, ch);
+ mutex_unlock(_tty_mutex);
}
static void spk_ttyio_tiocmset(unsigned int set, unsigned int clear)
{
+ mutex_lock(_tty_mutex);
+ if (check_tty(speakup_tty)) {
+ mutex_unlock(_tty_mutex);
+ return;
+ }
+
speakup_tty->ops->tiocmset(speakup_tty, set, clear);
+ mutex_unlock(_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(_tty_mutex);
+ if (check_tty(speakup_tty)) {
+ mutex_unlock(_tty_mutex);
+ return;
+ }
+
if (speakup_tty->ops->flush_buffer)
speakup_tty->ops->flush_buffer(speakup_tty);
+
+ mutex_unlock(_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 <okash.khaw...@gmail.com>
Reviewed-by: Samuel Thibault <samuel.thiba...@ens-lyon.org>
---
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);
}
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 <okash.khaw...@gmail.com>
---
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 <okash.khaw...@gmail.com> --- 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 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 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 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 <okash.khaw...@gmail.com>
---
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(>files_lock);
list_for_each(p, >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(_mutex);
+ tty_port_set_kopened(tty->port, 0);
+ release_tty(tty, tty->index);
+ mutex_unlock(_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 _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(_mutex);
+ driver = tty_lookup_driver(device, NULL, );
+ if (IS_ERR(driver)) {
+ mutex_unlock(_mutex);
+ return ERR_CAST(driver);
+ }
+
+ /* check wh
[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 <okash.khaw...@gmail.com>
---
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 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(>files_lock);
list_for_each(p, >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(_mutex);
+ tty_port_set_kopened(tty->port, 0);
+ release_tty(tty, tty->index);
+ mutex_unlock(_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 _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(_mutex);
+ driver = tty_lookup_driver(device, NULL, );
+ if (IS_ERR(driver)) {
+ mutex_unlock(_mutex);
+ return ERR_CAST(driver);
+ }
+
+ /* check whether we're reopening an
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 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(>files_lock);
list_for_each(p, >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 _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(_mutex);
+ driver = tty_lookup_driver(device, NULL, );
+ if (IS_ERR(driver)) {
+ mutex_unlock(_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(_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(_mutex);
+ tty = ERR_PTR(-EBUSY);
+ goto out;
+ }
mutex_unlock(_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 */
+#define TTY_PORT_KOPENED 5
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(>files_lock);
list_for_each(p, >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 _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(_mutex);
+ driver = tty_lookup_driver(device, NULL, );
+ if (IS_ERR(driver)) {
+ mutex_unlock(_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(_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(_mutex);
+ tty = ERR_PTR(-EBUSY);
+ goto out;
+ }
mutex_unlock(_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 */
+#define TTY_PORT_KOPENED 5
[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 <okash.khaw...@gmail.com> --- 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 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 <okash.khaw...@gmail.com>
---
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(>files_lock);
list_for_each(p, >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, >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 _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(_mutex);
+ driver = tty_lookup_driver(device, NULL, );
+ if (IS_ERR(driver)) {
+ mutex_unlock(_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, >flags);
+ }
+out:
+ mutex_unlock(_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, >flags)) {
+ tty_kref_put(tty);
+ mutex_unlock(_mutex);
+ tty = ERR_PTR(-EBUSY);
+ goto out;
+ }
mutex_unlock(_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
[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(>files_lock);
list_for_each(p, >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, >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 _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(_mutex);
+ driver = tty_lookup_driver(device, NULL, );
+ if (IS_ERR(driver)) {
+ mutex_unlock(_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, >flags);
+ }
+out:
+ mutex_unlock(_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, >flags)) {
+ tty_kref_put(tty);
+ mutex_unlock(_mutex);
+ tty = ERR_PTR(-EBUSY);
+ goto out;
+ }
mutex_unlock(_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
[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 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 <okash.khaw...@gmail.com>
---
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)
[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 <gre...@linuxfoundation.org> > 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
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 <okash.khaw...@gmail.com>
---
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(_mutex);
synth_release();
mutex_unlock(_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, _ttyio_ldisc_ops);
- if (ret) {
- pr_err("Error registering line discipline.\n");
- return ret;
- }
-
ret = get_dev_to_use(synth, );
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 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(_mutex);
synth_release();
mutex_unlock(_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, _ttyio_ldisc_ops);
- if (ret) {
- pr_err("Error registering line discipline.\n");
- return ret;
- }
-
ret = get_dev_to_use(synth, );
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 <okash.khaw...@gmail.com>
---
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, _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 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, _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 <okash.khaw...@gmail.com> --- 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);
[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 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
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 <okash.khaw...@gmail.com> 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] 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 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 <okash.khaw...@gmail.com>
> 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 <okash.khaw...@gmail.com>
> >> wrote:
> >>
> >> > +struct tty_struct *tty_kopen(dev_t device)
> >> > +{
> >> > + struct tty_struct *tty;
> >> > + struct tty_driver *driver = NULL;
> >> > + int index = -1;
> >> > +
> >> > + mutex_lock(_mutex);
> >> > + driver = tty_lookup_driver(device, NULL, );
> >> > + if (IS_ERR(driver)) {
> >>
> >> > + mutex_unlock(_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 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(_mutex);
> >> > + driver = tty_lookup_driver(device, NULL, );
> >> > + if (IS_ERR(driver)) {
> >>
> >> > + mutex_unlock(_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 <okash.khaw...@gmail.com> 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 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 <okash.khaw...@gmail.com> wrote:
>
> > +struct tty_struct *tty_kopen(dev_t device)
> > +{
> > + struct tty_struct *tty;
> > + struct tty_driver *driver = NULL;
> > + int index = -1;
> > +
> > + mutex_lock(_mutex);
> > + driver = tty_lookup_driver(device, NULL, );
> > + if (IS_ERR(driver)) {
>
> > + mutex_unlock(_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
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(_mutex);
> > + driver = tty_lookup_driver(device, NULL, );
> > + if (IS_ERR(driver)) {
>
> > + mutex_unlock(_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
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 <okash.khaw...@gmail.com>
---
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
[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 <okash.khaw...@gmail.com> > > > > --- > > 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
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 <okash.khaw...@gmail.com>
---
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 _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(_mutex);
+ driver = tty_lookup_driver(device, NULL, );
+ if (IS_ERR(driver)) {
+ mutex_unlock(_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, >flags);
+ }
+out:
+ mutex_unlock(_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, >flags)) {
+ tty_kref_put(tty);
+ mutex_unlock(_mutex);
+ tty = ERR_PTR(-EBUSY);
+ goto out;
+ }
mutex_unlock(_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 <okash.khaw...@gmail.com>
---
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 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 _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(_mutex);
+ driver = tty_lookup_driver(device, NULL, );
+ if (IS_ERR(driver)) {
+ mutex_unlock(_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, >flags);
+ }
+out:
+ mutex_unlock(_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, >flags)) {
+ tty_kref_put(tty);
+ mutex_unlock(_mutex);
+ tty = ERR_PTR(-EBUSY);
+ goto out;
+ }
mutex_unlock(_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)
