| Message ID | 20200723003357.26897-2-Sergey.Semin@baikalelectronics.ru (mailing list archive) |
|---|---|
| State | Mainlined |
| Commit | 868f3ee6e452bc2b89e68183a1700fcbbe0807b1 |
| Headers | show |
| Series | serial: 8250_dw: Fix ref clock usage | expand |
On Thu, Jul 23, 2020 at 03:33:54AM +0300, Serge Semin wrote: > Some platforms can be designed in a way so the UART port reference clock > might be asynchronously changed at some point. In Baikal-T1 SoC this may > happen due to the reference clock being shared between two UART ports, on > the Allwinner SoC the reference clock is derived from the CPU clock, so > any CPU frequency change should get to be known/reflected by/in the UART > controller as well. But it's not enough to just update the > uart_port->uartclk field of the corresponding UART port, the 8250 > controller reference clock divisor should be altered so to preserve > current baud rate setting. All of these things is done in a coherent > way by calling the serial8250_update_uartclk() method provided in this > patch. Though note that it isn't supposed to be called from within the > UART port callbacks because the locks using to the protect the UART port > data are already taken in there. ... > +/* > + * Note in order to avoid the tty port mutex deadlock don't use the next method > + * within the uart port callbacks. Primarily it's supposed to be utilized to > + * handle a sudden reference clock rate change. > + */ > +void serial8250_update_uartclk(struct uart_port *port, unsigned int uartclk) > +{ > + struct uart_8250_port *up = up_to_u8250p(port); > + unsigned int baud, quot, frac = 0; > + struct ktermios *termios; > + unsigned long flags; > + > + mutex_lock(&port->state->port.mutex); > + > + if (port->uartclk == uartclk) > + goto out_lock; > + > + port->uartclk = uartclk; > + termios = &port->state->port.tty->termios; > + > + baud = serial8250_get_baud_rate(port, termios, NULL); > + quot = serial8250_get_divisor(port, baud, &frac); > + > + serial8250_rpm_get(up); > + spin_lock_irqsave(&port->lock, flags); > + > + uart_update_timeout(port, termios->c_cflag, baud); > + > + serial8250_set_divisor(port, baud, quot, frac); > + serial_port_out(port, UART_LCR, up->lcr); > + serial8250_out_MCR(up, UART_MCR_DTR | UART_MCR_RTS); > + > + spin_unlock_irqrestore(&port->lock, flags); > + serial8250_rpm_put(up); > + > +out_lock: > + mutex_unlock(&port->state->port.mutex); While looking for something else I have stumbled over this function. My Q is, since it has some duplications with serial8250_do_set_termios(), can we actually call the latter (or derevative that can be called in both) in the above code instead of duplicating some lines? if (port UART clock has to be updated) call (unlocked version of) serial8250_do_set_termios() Serge, what do you think? > +}
Hi Andy, On Wed, Feb 14, 2024 at 03:45:16PM +0200, Andy Shevchenko wrote: > On Thu, Jul 23, 2020 at 03:33:54AM +0300, Serge Semin wrote: > > Some platforms can be designed in a way so the UART port reference clock > > might be asynchronously changed at some point. In Baikal-T1 SoC this may > > happen due to the reference clock being shared between two UART ports, on > > the Allwinner SoC the reference clock is derived from the CPU clock, so > > any CPU frequency change should get to be known/reflected by/in the UART > > controller as well. But it's not enough to just update the > > uart_port->uartclk field of the corresponding UART port, the 8250 > > controller reference clock divisor should be altered so to preserve > > current baud rate setting. All of these things is done in a coherent > > way by calling the serial8250_update_uartclk() method provided in this > > patch. Though note that it isn't supposed to be called from within the > > UART port callbacks because the locks using to the protect the UART port > > data are already taken in there. > > ... > > > +/* > > + * Note in order to avoid the tty port mutex deadlock don't use the next method > > + * within the uart port callbacks. Primarily it's supposed to be utilized to > > + * handle a sudden reference clock rate change. > > + */ > > +void serial8250_update_uartclk(struct uart_port *port, unsigned int uartclk) > > +{ > > + struct uart_8250_port *up = up_to_u8250p(port); > > + unsigned int baud, quot, frac = 0; > > + struct ktermios *termios; > > + unsigned long flags; > > + > > + mutex_lock(&port->state->port.mutex); > > + > > + if (port->uartclk == uartclk) > > + goto out_lock; > > + > > + port->uartclk = uartclk; > > + termios = &port->state->port.tty->termios; > > + > > + baud = serial8250_get_baud_rate(port, termios, NULL); > > + quot = serial8250_get_divisor(port, baud, &frac); > > + > > + serial8250_rpm_get(up); > > + spin_lock_irqsave(&port->lock, flags); > > + > > + uart_update_timeout(port, termios->c_cflag, baud); > > + > > + serial8250_set_divisor(port, baud, quot, frac); > > + serial_port_out(port, UART_LCR, up->lcr); > > + serial8250_out_MCR(up, UART_MCR_DTR | UART_MCR_RTS); > > + > > + spin_unlock_irqrestore(&port->lock, flags); > > + serial8250_rpm_put(up); > > + > > +out_lock: > > + mutex_unlock(&port->state->port.mutex); > > While looking for something else I have stumbled over this function. > My Q is, since it has some duplications with > serial8250_do_set_termios(), can we actually call the latter (or > derevative that can be called in both) in the above code instead of > duplicating some lines? > > if (port UART clock has to be updated) > call (unlocked version of) serial8250_do_set_termios() > > Serge, what do you think? What an old thread you've digged out.) Well, AFAIR I didn't create a common baud-rate/clock-update method because the baud-rate change was just a two stages action: 1. calculate divisor+quot couple based on the new clock, 2. update the divisor+quot (+ update the timeout). The first stage didn't need to have the IRQsafe lock being held and the runtime-PM being enabled, meanwhile the later one needed those. So unless the nested locking or try-lock-based pattern is implemented each stage required dedicated function introduced, which would have been an overkill for that. But even if I got to implement the try-lock-based solution with a single function containing both stages I still couldn't avoid having the serial8250_get_baud_rate() and serial8250_get_divisor() methods executed in the atomic context, which isn't required for them and which would needlessly pro-long the CPU executing with the IRQs disabled. As you well know it's better to speed up the atomic context execution as much as possible. Secondly I didn't know much about the tty/serial subsystem internals back then. So I was afraid to break some parts I didn't aware of if the baud-rate/ref-clock change code had some implicit dependencies from the surrounding code and vice-versa (like the LCR DLAB flag state). Finally frankly it didn't seem like that much worth bothering about. Basically AFAICS there were only four methods which invocation I would have needed to move to a separate function: serial8250_get_baud_rate(); serial8250_get_divisor(); // spin-lock uart_update_timeout(port, termios->c_cflag, baud); serial8250_set_divisor(port, baud, quot, frac); // spin-unlock So I decided to take a simplest and safest path, and created a dedicated method for the just the ref-clock updates case leaving the baud-rate change task implemented in the framework of the standard serial8250_do_set_termios() method. Regarding doing vise-versa and calling the serial8250_do_set_termios() method from serial8250_update_uartclk() instead. To be honest I didn't consider that option. That might work though, but AFAICS the serial8250_do_set_termios() function will do much more than it's required in case if the ref-clock has changed. -Serge(y) > > > +} > > -- > With Best Regards, > Andy Shevchenko > >
On Thu, Feb 15, 2024 at 10:32:18PM +0300, Serge Semin wrote: > On Wed, Feb 14, 2024 at 03:45:16PM +0200, Andy Shevchenko wrote: > > On Thu, Jul 23, 2020 at 03:33:54AM +0300, Serge Semin wrote: > > > Some platforms can be designed in a way so the UART port reference clock > > > might be asynchronously changed at some point. In Baikal-T1 SoC this may > > > happen due to the reference clock being shared between two UART ports, on > > > the Allwinner SoC the reference clock is derived from the CPU clock, so > > > any CPU frequency change should get to be known/reflected by/in the UART > > > controller as well. But it's not enough to just update the > > > uart_port->uartclk field of the corresponding UART port, the 8250 > > > controller reference clock divisor should be altered so to preserve > > > current baud rate setting. All of these things is done in a coherent > > > way by calling the serial8250_update_uartclk() method provided in this > > > patch. Though note that it isn't supposed to be called from within the > > > UART port callbacks because the locks using to the protect the UART port > > > data are already taken in there. ... > > > +/* > > > + * Note in order to avoid the tty port mutex deadlock don't use the next method > > > + * within the uart port callbacks. Primarily it's supposed to be utilized to > > > + * handle a sudden reference clock rate change. > > > + */ > > > +void serial8250_update_uartclk(struct uart_port *port, unsigned int uartclk) > > > +{ > > > + struct uart_8250_port *up = up_to_u8250p(port); > > > + unsigned int baud, quot, frac = 0; > > > + struct ktermios *termios; > > > + unsigned long flags; > > > + > > > + mutex_lock(&port->state->port.mutex); > > > + > > > + if (port->uartclk == uartclk) > > > + goto out_lock; > > > + > > > + port->uartclk = uartclk; > > > + termios = &port->state->port.tty->termios; > > > + > > > + baud = serial8250_get_baud_rate(port, termios, NULL); > > > + quot = serial8250_get_divisor(port, baud, &frac); > > > + > > > + serial8250_rpm_get(up); > > > + spin_lock_irqsave(&port->lock, flags); > > > + > > > + uart_update_timeout(port, termios->c_cflag, baud); > > > + > > > + serial8250_set_divisor(port, baud, quot, frac); > > > + serial_port_out(port, UART_LCR, up->lcr); > > > + serial8250_out_MCR(up, UART_MCR_DTR | UART_MCR_RTS); > > > + > > > + spin_unlock_irqrestore(&port->lock, flags); > > > + serial8250_rpm_put(up); > > > + > > > +out_lock: > > > + mutex_unlock(&port->state->port.mutex); > > > > > While looking for something else I have stumbled over this function. > > My Q is, since it has some duplications with > > serial8250_do_set_termios(), can we actually call the latter (or > > derevative that can be called in both) in the above code instead of > > duplicating some lines? > > > > if (port UART clock has to be updated) > > call (unlocked version of) serial8250_do_set_termios() > > > > Serge, what do you think? > > What an old thread you've digged out.) Indeed :-) > Well, AFAIR I didn't create a common baud-rate/clock-update method > because the baud-rate change was just a two stages action: > 1. calculate divisor+quot couple based on the new clock, > 2. update the divisor+quot (+ update the timeout). > The first stage didn't need to have the IRQsafe lock being held and > the runtime-PM being enabled, meanwhile the later one needed those. > So unless the nested locking or try-lock-based pattern is implemented > each stage required dedicated function introduced, which would have > been an overkill for that. But even if I got to implement the > try-lock-based solution with a single function containing both stages > I still couldn't avoid having the serial8250_get_baud_rate() and > serial8250_get_divisor() methods executed in the atomic context, which > isn't required for them and which would needlessly pro-long the CPU > executing with the IRQs disabled. As you well know it's better to > speed up the atomic context execution as much as possible. > > Secondly I didn't know much about the tty/serial subsystem internals > back then. So I was afraid to break some parts I didn't aware of if > the baud-rate/ref-clock change code had some implicit dependencies > from the surrounding code and vice-versa (like the LCR DLAB flag > state). > > Finally frankly it didn't seem like that much worth bothering about. > Basically AFAICS there were only four methods which invocation I > would have needed to move to a separate function: > > serial8250_get_baud_rate(); > serial8250_get_divisor(); > // spin-lock > uart_update_timeout(port, termios->c_cflag, baud); > serial8250_set_divisor(port, baud, quot, frac); > // spin-unlock > > So I decided to take a simplest and safest path, and created a > dedicated method for the just the ref-clock updates case leaving the > baud-rate change task implemented in the framework of the standard > serial8250_do_set_termios() method. > > > Regarding doing vise-versa and calling the serial8250_do_set_termios() > method from serial8250_update_uartclk() instead. To be honest I didn't > consider that option. That might work though, but AFAICS the > serial8250_do_set_termios() function will do much more than it's > required in case if the ref-clock has changed. My point here is that the idea behind clock change is most likely to be followed up by ->set_termios(). Why to do it differently if it's the case? And note, ->set_termios() can be called as many times as needed, so if nothing changes in between it's also fine. But this makes intention much clearer. Do you agree?
On Thu, Feb 15, 2024 at 09:39:12PM +0200, Andy Shevchenko wrote: > On Thu, Feb 15, 2024 at 10:32:18PM +0300, Serge Semin wrote: > > On Wed, Feb 14, 2024 at 03:45:16PM +0200, Andy Shevchenko wrote: > > > On Thu, Jul 23, 2020 at 03:33:54AM +0300, Serge Semin wrote: > > > > Some platforms can be designed in a way so the UART port reference clock > > > > might be asynchronously changed at some point. In Baikal-T1 SoC this may > > > > happen due to the reference clock being shared between two UART ports, on > > > > the Allwinner SoC the reference clock is derived from the CPU clock, so > > > > any CPU frequency change should get to be known/reflected by/in the UART > > > > controller as well. But it's not enough to just update the > > > > uart_port->uartclk field of the corresponding UART port, the 8250 > > > > controller reference clock divisor should be altered so to preserve > > > > current baud rate setting. All of these things is done in a coherent > > > > way by calling the serial8250_update_uartclk() method provided in this > > > > patch. Though note that it isn't supposed to be called from within the > > > > UART port callbacks because the locks using to the protect the UART port > > > > data are already taken in there. > > ... > > > > > +/* > > > > + * Note in order to avoid the tty port mutex deadlock don't use the next method > > > > + * within the uart port callbacks. Primarily it's supposed to be utilized to > > > > + * handle a sudden reference clock rate change. > > > > + */ > > > > +void serial8250_update_uartclk(struct uart_port *port, unsigned int uartclk) > > > > +{ > > > > + struct uart_8250_port *up = up_to_u8250p(port); > > > > + unsigned int baud, quot, frac = 0; > > > > + struct ktermios *termios; > > > > + unsigned long flags; > > > > + > > > > + mutex_lock(&port->state->port.mutex); > > > > + > > > > + if (port->uartclk == uartclk) > > > > + goto out_lock; > > > > + > > > > + port->uartclk = uartclk; > > > > + termios = &port->state->port.tty->termios; > > > > + > > > > + baud = serial8250_get_baud_rate(port, termios, NULL); > > > > + quot = serial8250_get_divisor(port, baud, &frac); > > > > + > > > > + serial8250_rpm_get(up); > > > > + spin_lock_irqsave(&port->lock, flags); > > > > + > > > > + uart_update_timeout(port, termios->c_cflag, baud); > > > > + > > > > + serial8250_set_divisor(port, baud, quot, frac); > > > > + serial_port_out(port, UART_LCR, up->lcr); > > > > + serial8250_out_MCR(up, UART_MCR_DTR | UART_MCR_RTS); > > > > + > > > > + spin_unlock_irqrestore(&port->lock, flags); > > > > + serial8250_rpm_put(up); > > > > + > > > > +out_lock: > > > > + mutex_unlock(&port->state->port.mutex); > > > > > > > > While looking for something else I have stumbled over this function. > > > My Q is, since it has some duplications with > > > serial8250_do_set_termios(), can we actually call the latter (or > > > derevative that can be called in both) in the above code instead of > > > duplicating some lines? > > > > > > if (port UART clock has to be updated) > > > call (unlocked version of) serial8250_do_set_termios() > > > > > > Serge, what do you think? > > > > What an old thread you've digged out.) > > Indeed :-) > > > Well, AFAIR I didn't create a common baud-rate/clock-update method > > because the baud-rate change was just a two stages action: > > 1. calculate divisor+quot couple based on the new clock, > > 2. update the divisor+quot (+ update the timeout). > > The first stage didn't need to have the IRQsafe lock being held and > > the runtime-PM being enabled, meanwhile the later one needed those. > > So unless the nested locking or try-lock-based pattern is implemented > > each stage required dedicated function introduced, which would have > > been an overkill for that. But even if I got to implement the > > try-lock-based solution with a single function containing both stages > > I still couldn't avoid having the serial8250_get_baud_rate() and > > serial8250_get_divisor() methods executed in the atomic context, which > > isn't required for them and which would needlessly pro-long the CPU > > executing with the IRQs disabled. As you well know it's better to > > speed up the atomic context execution as much as possible. > > > > Secondly I didn't know much about the tty/serial subsystem internals > > back then. So I was afraid to break some parts I didn't aware of if > > the baud-rate/ref-clock change code had some implicit dependencies > > from the surrounding code and vice-versa (like the LCR DLAB flag > > state). > > > > Finally frankly it didn't seem like that much worth bothering about. > > Basically AFAICS there were only four methods which invocation I > > would have needed to move to a separate function: > > > > serial8250_get_baud_rate(); > > serial8250_get_divisor(); > > // spin-lock > > uart_update_timeout(port, termios->c_cflag, baud); > > serial8250_set_divisor(port, baud, quot, frac); > > // spin-unlock > > > > So I decided to take a simplest and safest path, and created a > > dedicated method for the just the ref-clock updates case leaving the > > baud-rate change task implemented in the framework of the standard > > serial8250_do_set_termios() method. > > > > > > Regarding doing vise-versa and calling the serial8250_do_set_termios() > > method from serial8250_update_uartclk() instead. To be honest I didn't > > consider that option. That might work though, but AFAICS the > > serial8250_do_set_termios() function will do much more than it's > > required in case if the ref-clock has changed. > > My point here is that the idea behind clock change is most likely to be > followed up by ->set_termios(). Why to do it differently if it's the case? Not always. IIUC what you say is just a one path of the code executed within the chain: dw8250_set_termios()->dw8250_do_set_termios()->serial8250_do_set_termios() But another code-path will be taken if the DW UART port ref-clock is suddenly and asynchronously changed. In that case the driver is notified by means of the dw8250_clk_notifier_cb() callback, which doesn't need the entire set_termios() callback execution but only what is defined in the serial8250_update_uartclk() method: dw8250_clk_notifier_cb() +-> worker:: dw8250_clk_work_cb()->serial8250_update_uartclk(). > And note, ->set_termios() can be called as many times as needed, so if nothing > changes in between it's also fine. But this makes intention much clearer. > Do you agree? If what you suggest is to replace the serial8250_update_uartclk() body with a direct uart_port::set_termios() invocation then I don't find it being much clearer really. The serial8250_update_uartclk() is currently specialized on doing one thing: adjusting the divider in case of the UART-clock change. If instead the entire serial8250_set_termios() method is called then for a reader it won't be easy to understand what is really required for a 8250 serial port to perceive the ref-clock change. But from the maintainability point of view I guess that it might be safer to just call serial8250_set_termios() indeed, since among the other things the later method implies the divider update too. Thus the maintainer won't need to support the two clock divider update implementations. From that perspective I agree, directly calling serial8250_set_termios() might be more suitable despite of it' doing more than required. -Serge(y) > > -- > With Best Regards, > Andy Shevchenko > >
On Fri, Feb 16, 2024 at 08:19:37PM +0300, Serge Semin wrote: > On Thu, Feb 15, 2024 at 09:39:12PM +0200, Andy Shevchenko wrote: ... (thanks for the detailed explanation why you have done it that way) > If what you suggest is to replace the serial8250_update_uartclk() body > with a direct uart_port::set_termios() invocation then I don't find it > being much clearer really. The serial8250_update_uartclk() is > currently specialized on doing one thing: adjusting the divider in > case of the UART-clock change. If instead the entire > serial8250_set_termios() method is called then for a reader it won't > be easy to understand what is really required for a 8250 serial port > to perceive the ref-clock change. But from the maintainability point > of view I guess that it might be safer to just call > serial8250_set_termios() indeed, since among the other things the > later method implies the divider update too. Thus the maintainer won't > need to support the two clock divider update implementations. > From that perspective I agree, directly calling serial8250_set_termios() > might be more suitable despite of it' doing more than required. Would it be possible for you to cook the patch (and test on your HW, since it seems the only user of that)?
On Mon, Feb 19, 2024 at 05:08:54PM +0200, Andy Shevchenko wrote: > On Fri, Feb 16, 2024 at 08:19:37PM +0300, Serge Semin wrote: > > On Thu, Feb 15, 2024 at 09:39:12PM +0200, Andy Shevchenko wrote: > > ... > > (thanks for the detailed explanation why you have done it that way) > > > If what you suggest is to replace the serial8250_update_uartclk() body > > with a direct uart_port::set_termios() invocation then I don't find it > > being much clearer really. The serial8250_update_uartclk() is > > currently specialized on doing one thing: adjusting the divider in > > case of the UART-clock change. If instead the entire > > serial8250_set_termios() method is called then for a reader it won't > > be easy to understand what is really required for a 8250 serial port > > to perceive the ref-clock change. But from the maintainability point > > of view I guess that it might be safer to just call > > serial8250_set_termios() indeed, since among the other things the > > later method implies the divider update too. Thus the maintainer won't > > need to support the two clock divider update implementations. > > > From that perspective I agree, directly calling serial8250_set_termios() > > might be more suitable despite of it' doing more than required. > > Would it be possible for you to cook the patch (and test on your HW, > since it seems the only user of that)? Agreed. The patch should have been just landed on your work and private inboxes. Link: https://lore.kernel.org/linux-serial/20240222145058.28307-1-fancer.lancer@gmail.com -Serge(y) > > -- > With Best Regards, > Andy Shevchenko > >
diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c index 1632f7d25acc..f19757ef4999 100644 --- a/drivers/tty/serial/8250/8250_port.c +++ b/drivers/tty/serial/8250/8250_port.c @@ -2631,6 +2631,46 @@ static unsigned int serial8250_get_baud_rate(struct uart_port *port, (port->uartclk + tolerance) / 16); } +/* + * Note in order to avoid the tty port mutex deadlock don't use the next method + * within the uart port callbacks. Primarily it's supposed to be utilized to + * handle a sudden reference clock rate change. + */ +void serial8250_update_uartclk(struct uart_port *port, unsigned int uartclk) +{ + struct uart_8250_port *up = up_to_u8250p(port); + unsigned int baud, quot, frac = 0; + struct ktermios *termios; + unsigned long flags; + + mutex_lock(&port->state->port.mutex); + + if (port->uartclk == uartclk) + goto out_lock; + + port->uartclk = uartclk; + termios = &port->state->port.tty->termios; + + baud = serial8250_get_baud_rate(port, termios, NULL); + quot = serial8250_get_divisor(port, baud, &frac); + + serial8250_rpm_get(up); + spin_lock_irqsave(&port->lock, flags); + + uart_update_timeout(port, termios->c_cflag, baud); + + serial8250_set_divisor(port, baud, quot, frac); + serial_port_out(port, UART_LCR, up->lcr); + serial8250_out_MCR(up, UART_MCR_DTR | UART_MCR_RTS); + + spin_unlock_irqrestore(&port->lock, flags); + serial8250_rpm_put(up); + +out_lock: + mutex_unlock(&port->state->port.mutex); +} +EXPORT_SYMBOL_GPL(serial8250_update_uartclk); + void serial8250_do_set_termios(struct uart_port *port, struct ktermios *termios, struct ktermios *old) diff --git a/include/linux/serial_8250.h b/include/linux/serial_8250.h index 6545f8cfc8fa..2b70f736b091 100644 --- a/include/linux/serial_8250.h +++ b/include/linux/serial_8250.h @@ -155,6 +155,8 @@ extern int early_serial_setup(struct uart_port *port); extern int early_serial8250_setup(struct earlycon_device *device, const char *options); +extern void serial8250_update_uartclk(struct uart_port *port, + unsigned int uartclk); extern void serial8250_do_set_termios(struct uart_port *port, struct ktermios *termios, struct ktermios *old); extern void serial8250_do_set_ldisc(struct uart_port *port,
Some platforms can be designed in a way so the UART port reference clock might be asynchronously changed at some point. In Baikal-T1 SoC this may happen due to the reference clock being shared between two UART ports, on the Allwinner SoC the reference clock is derived from the CPU clock, so any CPU frequency change should get to be known/reflected by/in the UART controller as well. But it's not enough to just update the uart_port->uartclk field of the corresponding UART port, the 8250 controller reference clock divisor should be altered so to preserve current baud rate setting. All of these things is done in a coherent way by calling the serial8250_update_uartclk() method provided in this patch. Though note that it isn't supposed to be called from within the UART port callbacks because the locks using to the protect the UART port data are already taken in there. Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru> --- Changelog v4: - Export serial8250_update_uartclk() symbol for GPL modules only. Changelog v7: - Wake the device up on the serial port divider update. --- drivers/tty/serial/8250/8250_port.c | 40 +++++++++++++++++++++++++++++ include/linux/serial_8250.h | 2 ++ 2 files changed, 42 insertions(+)