adjtimex broken in 3.12.37 - 3.12.39
Hello Jiri,
Commit f09c62a1c27dcc43a06e5ebb27ae67306146277b ("time: adjtimex:
Validate the ADJ_FREQUENCY values", appeared first in 3.12.37,
upstream is 5e5aeb4367b450a28f447f6d5ab57d8f2ab16a5f) breaks the
adjtimex call on 32 bit systems. This was fixed upstream by commit
29183a70b0b828500816bd794b3fe192fce89f73 ("ntp: Fixup adjtimex freq
validation on 32-bit systems").
If you haven't already done this, please consider applying the fix to
the 3.12 series. Thank you!
Christian
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
adjtimex broken in 3.12.37 - 3.12.39
Hello Jiri, Commit f09c62a1c27dcc43a06e5ebb27ae67306146277b (time: adjtimex: Validate the ADJ_FREQUENCY values, appeared first in 3.12.37, upstream is 5e5aeb4367b450a28f447f6d5ab57d8f2ab16a5f) breaks the adjtimex call on 32 bit systems. This was fixed upstream by commit 29183a70b0b828500816bd794b3fe192fce89f73 (ntp: Fixup adjtimex freq validation on 32-bit systems). If you haven't already done this, please consider applying the fix to the 3.12 series. Thank you! Christian -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] pps: Add support for read operations and return a useful value in poll
[sent again with Rodolfo in the list of recipients, something went wrong, sorry!] Hi Rodolfo, Thanks for your reply! On Wed, Apr 1, 2015 at 10:11 AM, Rodolfo Giometti wrote: > On Tue, Mar 31, 2015 at 11:31:22PM +0200, Christian Riesch wrote: >> The PPS_FETCH ioctl in drivers/pps/pps.c blocks until a new PPS event >> occurs, then returns the time stamp data. While this is fine for >> lots of applications, sometimes it would be nice if the poll system >> call and a subsequent read could be used to obtain the pps data. > > Nak. The read syscall can't be forced to return fix amount of > data. I just copied/modified the behavior of ptp_read() in drivers/ptp/ptp_chardev.c: if (cnt % sizeof(struct ptp_extts_event) != 0) return -EINVAL; There the amount of data is forced to be a multiple of sizeof(struct ptp_extts_event). But if you prefer an ioctl, I can change that of course. > Use a dedicated ioctl instead. Is it ok to pair POLLIN | POLLRDNORM in the poll function with an ioctl? Or should returning POLLIN | POLLRDNORM mean that a read() will not block? Should I use POLLPRI instead or something else? Regards, Christian -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] pps: Add support for read operations and return a useful value in poll
Hi Rodolfo, Thanks for your reply! On Wed, Apr 1, 2015 at 10:11 AM, Rodolfo Giometti wrote: > On Tue, Mar 31, 2015 at 11:31:22PM +0200, Christian Riesch wrote: >> The PPS_FETCH ioctl in drivers/pps/pps.c blocks until a new PPS event >> occurs, then returns the time stamp data. While this is fine for >> lots of applications, sometimes it would be nice if the poll system >> call and a subsequent read could be used to obtain the pps data. > > Nak. The read syscall can't be forced to return fix amount of > data. I just copied/modified the behavior of ptp_read() in drivers/ptp/ptp_chardev.c: if (cnt % sizeof(struct ptp_extts_event) != 0) return -EINVAL; There the amount of data is forced to be a multiple of sizeof(struct ptp_extts_event). But if you prefer an ioctl, I can change that of course. > > Use a dedicated ioctl instead. Is it ok to pair POLLIN | POLLRDNORM in the poll function with an ioctl? Or should returning POLLIN | POLLRDNORM mean that a read() will not block? Should I use POLLPRI instead or something else? Regards, Christian -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] pps: Add support for read operations and return a useful value in poll
[sent again with Rodolfo in the list of recipients, something went wrong, sorry!] Hi Rodolfo, Thanks for your reply! On Wed, Apr 1, 2015 at 10:11 AM, Rodolfo Giometti giome...@enneenne.com wrote: On Tue, Mar 31, 2015 at 11:31:22PM +0200, Christian Riesch wrote: The PPS_FETCH ioctl in drivers/pps/pps.c blocks until a new PPS event occurs, then returns the time stamp data. While this is fine for lots of applications, sometimes it would be nice if the poll system call and a subsequent read could be used to obtain the pps data. Nak. The read syscall can't be forced to return fix amount of data. I just copied/modified the behavior of ptp_read() in drivers/ptp/ptp_chardev.c: if (cnt % sizeof(struct ptp_extts_event) != 0) return -EINVAL; There the amount of data is forced to be a multiple of sizeof(struct ptp_extts_event). But if you prefer an ioctl, I can change that of course. Use a dedicated ioctl instead. Is it ok to pair POLLIN | POLLRDNORM in the poll function with an ioctl? Or should returning POLLIN | POLLRDNORM mean that a read() will not block? Should I use POLLPRI instead or something else? Regards, Christian -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] pps: Add support for read operations and return a useful value in poll
Hi Rodolfo, Thanks for your reply! On Wed, Apr 1, 2015 at 10:11 AM, Rodolfo Giometti giome...@enneenne.com wrote: On Tue, Mar 31, 2015 at 11:31:22PM +0200, Christian Riesch wrote: The PPS_FETCH ioctl in drivers/pps/pps.c blocks until a new PPS event occurs, then returns the time stamp data. While this is fine for lots of applications, sometimes it would be nice if the poll system call and a subsequent read could be used to obtain the pps data. Nak. The read syscall can't be forced to return fix amount of data. I just copied/modified the behavior of ptp_read() in drivers/ptp/ptp_chardev.c: if (cnt % sizeof(struct ptp_extts_event) != 0) return -EINVAL; There the amount of data is forced to be a multiple of sizeof(struct ptp_extts_event). But if you prefer an ioctl, I can change that of course. Use a dedicated ioctl instead. Is it ok to pair POLLIN | POLLRDNORM in the poll function with an ioctl? Or should returning POLLIN | POLLRDNORM mean that a read() will not block? Should I use POLLPRI instead or something else? Regards, Christian -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] pps: Add support for read operations and return a useful value in poll
The PPS_FETCH ioctl in drivers/pps/pps.c blocks until a new PPS event
occurs, then returns the time stamp data. While this is fine for
lots of applications, sometimes it would be nice if the poll system
call and a subsequent read could be used to obtain the pps data.
This patch adds support for read operations. Furthermore pps_cdev_poll
is modified to return POLLIN | POLLRDNORM only when new PPS data is
available.
Signed-off-by: Christian Riesch
---
drivers/pps/pps.c | 32 +++-
include/linux/pps_kernel.h |1 +
2 files changed, 32 insertions(+), 1 deletion(-)
diff --git a/drivers/pps/pps.c b/drivers/pps/pps.c
index 2f07cd6..f90dd2e 100644
--- a/drivers/pps/pps.c
+++ b/drivers/pps/pps.c
@@ -55,7 +55,7 @@ static unsigned int pps_cdev_poll(struct file *file,
poll_table *wait)
poll_wait(file, >queue, wait);
- return POLLIN | POLLRDNORM;
+ return (pps->last_ev != pps->read_ev) ? POLLIN | POLLRDNORM : 0;
}
static int pps_cdev_fasync(int fd, struct file *file, int on)
@@ -191,6 +191,7 @@ static long pps_cdev_ioctl(struct file *file,
fdata.info.assert_tu = pps->assert_tu;
fdata.info.clear_tu = pps->clear_tu;
fdata.info.current_mode = pps->current_mode;
+ pps->read_ev = pps->last_ev;
spin_unlock_irq(>lock);
@@ -251,6 +252,34 @@ static int pps_cdev_open(struct inode *inode, struct file
*file)
return 0;
}
+ssize_t pps_cdev_read(struct file *file, char __user *buf, size_t count,
+ loff_t *ppos)
+{
+ struct pps_device *pps = file->private_data;
+ struct pps_fdata fdata;
+
+ if (count != sizeof(struct pps_fdata))
+ return -EINVAL;
+
+ if (wait_event_interruptible(pps->queue, pps->read_ev != pps->last_ev))
+ return -ERESTARTSYS;
+
+ /* Return the fetched timestamp */
+ spin_lock_irq(>lock);
+ fdata.info.assert_sequence = pps->assert_sequence;
+ fdata.info.clear_sequence = pps->clear_sequence;
+ fdata.info.assert_tu = pps->assert_tu;
+ fdata.info.clear_tu = pps->clear_tu;
+ fdata.info.current_mode = pps->current_mode;
+ pps->read_ev = pps->last_ev;
+ spin_unlock_irq(>lock);
+
+ if (copy_to_user(buf, , sizeof(struct pps_fdata)))
+ return -EFAULT;
+
+ return sizeof(struct pps_fdata);
+}
+
static int pps_cdev_release(struct inode *inode, struct file *file)
{
struct pps_device *pps = container_of(inode->i_cdev,
@@ -266,6 +295,7 @@ static int pps_cdev_release(struct inode *inode, struct
file *file)
static const struct file_operations pps_cdev_fops = {
.owner = THIS_MODULE,
.llseek = no_llseek,
+ .read = pps_cdev_read,
.poll = pps_cdev_poll,
.fasync = pps_cdev_fasync,
.unlocked_ioctl = pps_cdev_ioctl,
diff --git a/include/linux/pps_kernel.h b/include/linux/pps_kernel.h
index 1d2cd21..42f405b 100644
--- a/include/linux/pps_kernel.h
+++ b/include/linux/pps_kernel.h
@@ -66,6 +66,7 @@ struct pps_device {
int current_mode; /* PPS mode at event time */
unsigned int last_ev; /* last PPS event id */
+ unsigned int read_ev; /* last PPS event read by user
*/
wait_queue_head_t queue;/* PPS event queue */
unsigned int id;/* PPS source unique ID */
--
1.7.9.5
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] pps: Add support for read operations and return a useful value in poll
The PPS_FETCH ioctl in drivers/pps/pps.c blocks until a new PPS event
occurs, then returns the time stamp data. While this is fine for
lots of applications, sometimes it would be nice if the poll system
call and a subsequent read could be used to obtain the pps data.
This patch adds support for read operations. Furthermore pps_cdev_poll
is modified to return POLLIN | POLLRDNORM only when new PPS data is
available.
Signed-off-by: Christian Riesch christian.rie...@omicron.at
---
drivers/pps/pps.c | 32 +++-
include/linux/pps_kernel.h |1 +
2 files changed, 32 insertions(+), 1 deletion(-)
diff --git a/drivers/pps/pps.c b/drivers/pps/pps.c
index 2f07cd6..f90dd2e 100644
--- a/drivers/pps/pps.c
+++ b/drivers/pps/pps.c
@@ -55,7 +55,7 @@ static unsigned int pps_cdev_poll(struct file *file,
poll_table *wait)
poll_wait(file, pps-queue, wait);
- return POLLIN | POLLRDNORM;
+ return (pps-last_ev != pps-read_ev) ? POLLIN | POLLRDNORM : 0;
}
static int pps_cdev_fasync(int fd, struct file *file, int on)
@@ -191,6 +191,7 @@ static long pps_cdev_ioctl(struct file *file,
fdata.info.assert_tu = pps-assert_tu;
fdata.info.clear_tu = pps-clear_tu;
fdata.info.current_mode = pps-current_mode;
+ pps-read_ev = pps-last_ev;
spin_unlock_irq(pps-lock);
@@ -251,6 +252,34 @@ static int pps_cdev_open(struct inode *inode, struct file
*file)
return 0;
}
+ssize_t pps_cdev_read(struct file *file, char __user *buf, size_t count,
+ loff_t *ppos)
+{
+ struct pps_device *pps = file-private_data;
+ struct pps_fdata fdata;
+
+ if (count != sizeof(struct pps_fdata))
+ return -EINVAL;
+
+ if (wait_event_interruptible(pps-queue, pps-read_ev != pps-last_ev))
+ return -ERESTARTSYS;
+
+ /* Return the fetched timestamp */
+ spin_lock_irq(pps-lock);
+ fdata.info.assert_sequence = pps-assert_sequence;
+ fdata.info.clear_sequence = pps-clear_sequence;
+ fdata.info.assert_tu = pps-assert_tu;
+ fdata.info.clear_tu = pps-clear_tu;
+ fdata.info.current_mode = pps-current_mode;
+ pps-read_ev = pps-last_ev;
+ spin_unlock_irq(pps-lock);
+
+ if (copy_to_user(buf, fdata, sizeof(struct pps_fdata)))
+ return -EFAULT;
+
+ return sizeof(struct pps_fdata);
+}
+
static int pps_cdev_release(struct inode *inode, struct file *file)
{
struct pps_device *pps = container_of(inode-i_cdev,
@@ -266,6 +295,7 @@ static int pps_cdev_release(struct inode *inode, struct
file *file)
static const struct file_operations pps_cdev_fops = {
.owner = THIS_MODULE,
.llseek = no_llseek,
+ .read = pps_cdev_read,
.poll = pps_cdev_poll,
.fasync = pps_cdev_fasync,
.unlocked_ioctl = pps_cdev_ioctl,
diff --git a/include/linux/pps_kernel.h b/include/linux/pps_kernel.h
index 1d2cd21..42f405b 100644
--- a/include/linux/pps_kernel.h
+++ b/include/linux/pps_kernel.h
@@ -66,6 +66,7 @@ struct pps_device {
int current_mode; /* PPS mode at event time */
unsigned int last_ev; /* last PPS event id */
+ unsigned int read_ev; /* last PPS event read by user
*/
wait_queue_head_t queue;/* PPS event queue */
unsigned int id;/* PPS source unique ID */
--
1.7.9.5
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] n_tty: Fix unordered accesses to lockless read buffer
On Thu, Jan 1, 2015 at 12:00 PM, Christian Riesch >> @@ -164,15
+160,17 @@ static inline int tty_put_user(struct tty_struct *tty,
unsigned char x,
>> static int receive_room(struct tty_struct *tty)
>> {
>> struct n_tty_data *ldata = tty->disc_data;
>> + size_t head = ACCESS_ONCE(ldata->commit_head);
>> + size_t tail = ACCESS_ONCE(ldata->read_tail);
>> int left;
>>
>> if (I_PARMRK(tty)) {
>> - /* Multiply read_cnt by 3, since each byte might take up to
>> + /* Multiply count by 3, since each byte might take up to
>> * three times as many spaces when PARMRK is set (depending
>> on
>> * its flags, e.g. parity error). */
>> - left = N_TTY_BUF_SIZE - read_cnt(ldata) * 3 - 1;
>> + left = N_TTY_BUF_SIZE - (head - tail) * 3 - 1;
>> } else
>> - left = N_TTY_BUF_SIZE - read_cnt(ldata) - 1;
>> + left = N_TTY_BUF_SIZE - (head - tail) - 1;
>
> Actually, less room may be available, if read_head != commit_head.
> Could this cause problems? I guess yes, at least in
> n_tty_receive_buf_common, where this could lead to a buffer overflow,
> right?
Sorry, should not be a problem, at least not for
n_tty_receive_buf_common, since this is producer path, right? But how
about the other calls of receive_room()?
Christian
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] n_tty: Fix unordered accesses to lockless read buffer
Peter,
Thank you for this patch! Unfortunately I had not much time for this
since my last patch submission, so I am happy that someone continued
this work.
I have a few comments/questions, please see below.
On Tue, Dec 30, 2014 at 1:05 PM, Peter Hurley wrote:
> Add commit_head buffer index, which the producer-side publishes
> after input processing. This ensures the consumer-side observes
> correctly-ordered writes in raw mode
I understand that the commit_head reduces the number of memory
barriers and makes some things easier. But what is so special about
raw mode that requires the introduction of commit_head?
> (ie., the buffer data is
> written before the buffer index is advanced).
>
> Further, remove read_cnt() and expand inline, using ACCESS_ONCE()
"ACCESS_ONCE() and memory barriers"?
> on the relevant buffer index; read_tail from the producer-side
> and canon_head/commit_head from the consumer-side, or both in shared
> paths such as receive_room().
>
> Based on work by Christian Riesch
>
> NB: Exclusive access is still guaranteed with termios_rwsem write
> lock, eg. by n_tty_set_termios() and in n_tty_ioctl(); in this
> circumstance, commit_head is equivalent to read_head.
>
> Cc: Christian Riesch
> Cc: # v3.14.x (will need backport to v3.12.x)
> Signed-off-by: Peter Hurley
> ---
> drivers/tty/n_tty.c | 80
> ++---
> 1 file changed, 40 insertions(+), 40 deletions(-)
>
> diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
> index d2b4967..a618b10 100644
> --- a/drivers/tty/n_tty.c
> +++ b/drivers/tty/n_tty.c
> @@ -90,6 +90,7 @@
> struct n_tty_data {
> /* producer-published */
> size_t read_head;
> + size_t commit_head; /* == read_head when not receiving */
> size_t canon_head;
> size_t echo_head;
> size_t echo_commit;
> @@ -127,11 +128,6 @@ struct n_tty_data {
> struct mutex output_lock;
> };
>
> -static inline size_t read_cnt(struct n_tty_data *ldata)
> -{
> - return ldata->read_head - ldata->read_tail;
> -}
> -
> static inline unsigned char read_buf(struct n_tty_data *ldata, size_t i)
> {
> return ldata->read_buf[i & (N_TTY_BUF_SIZE - 1)];
> @@ -164,15 +160,17 @@ static inline int tty_put_user(struct tty_struct *tty,
> unsigned char x,
> static int receive_room(struct tty_struct *tty)
> {
> struct n_tty_data *ldata = tty->disc_data;
> + size_t head = ACCESS_ONCE(ldata->commit_head);
> + size_t tail = ACCESS_ONCE(ldata->read_tail);
> int left;
>
> if (I_PARMRK(tty)) {
> - /* Multiply read_cnt by 3, since each byte might take up to
> + /* Multiply count by 3, since each byte might take up to
> * three times as many spaces when PARMRK is set (depending on
> * its flags, e.g. parity error). */
> - left = N_TTY_BUF_SIZE - read_cnt(ldata) * 3 - 1;
> + left = N_TTY_BUF_SIZE - (head - tail) * 3 - 1;
> } else
> - left = N_TTY_BUF_SIZE - read_cnt(ldata) - 1;
> + left = N_TTY_BUF_SIZE - (head - tail) - 1;
Actually, less room may be available, if read_head != commit_head.
Could this cause problems? I guess yes, at least in
n_tty_receive_buf_common, where this could lead to a buffer overflow,
right?
Best regards,
Christian
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] n_tty: Fix unordered accesses to lockless read buffer
Peter,
Thank you for this patch! Unfortunately I had not much time for this
since my last patch submission, so I am happy that someone continued
this work.
I have a few comments/questions, please see below.
On Tue, Dec 30, 2014 at 1:05 PM, Peter Hurley pe...@hurleysoftware.com wrote:
Add commit_head buffer index, which the producer-side publishes
after input processing. This ensures the consumer-side observes
correctly-ordered writes in raw mode
I understand that the commit_head reduces the number of memory
barriers and makes some things easier. But what is so special about
raw mode that requires the introduction of commit_head?
(ie., the buffer data is
written before the buffer index is advanced).
Further, remove read_cnt() and expand inline, using ACCESS_ONCE()
ACCESS_ONCE() and memory barriers?
on the relevant buffer index; read_tail from the producer-side
and canon_head/commit_head from the consumer-side, or both in shared
paths such as receive_room().
Based on work by Christian Riesch christian.rie...@omicron.at
NB: Exclusive access is still guaranteed with termios_rwsem write
lock, eg. by n_tty_set_termios() and in n_tty_ioctl(); in this
circumstance, commit_head is equivalent to read_head.
Cc: Christian Riesch christian.rie...@omicron.at
Cc: sta...@vger.kernel.org # v3.14.x (will need backport to v3.12.x)
Signed-off-by: Peter Hurley pe...@hurleysoftware.com
---
drivers/tty/n_tty.c | 80
++---
1 file changed, 40 insertions(+), 40 deletions(-)
diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
index d2b4967..a618b10 100644
--- a/drivers/tty/n_tty.c
+++ b/drivers/tty/n_tty.c
@@ -90,6 +90,7 @@
struct n_tty_data {
/* producer-published */
size_t read_head;
+ size_t commit_head; /* == read_head when not receiving */
size_t canon_head;
size_t echo_head;
size_t echo_commit;
@@ -127,11 +128,6 @@ struct n_tty_data {
struct mutex output_lock;
};
-static inline size_t read_cnt(struct n_tty_data *ldata)
-{
- return ldata-read_head - ldata-read_tail;
-}
-
static inline unsigned char read_buf(struct n_tty_data *ldata, size_t i)
{
return ldata-read_buf[i (N_TTY_BUF_SIZE - 1)];
@@ -164,15 +160,17 @@ static inline int tty_put_user(struct tty_struct *tty,
unsigned char x,
static int receive_room(struct tty_struct *tty)
{
struct n_tty_data *ldata = tty-disc_data;
+ size_t head = ACCESS_ONCE(ldata-commit_head);
+ size_t tail = ACCESS_ONCE(ldata-read_tail);
int left;
if (I_PARMRK(tty)) {
- /* Multiply read_cnt by 3, since each byte might take up to
+ /* Multiply count by 3, since each byte might take up to
* three times as many spaces when PARMRK is set (depending on
* its flags, e.g. parity error). */
- left = N_TTY_BUF_SIZE - read_cnt(ldata) * 3 - 1;
+ left = N_TTY_BUF_SIZE - (head - tail) * 3 - 1;
} else
- left = N_TTY_BUF_SIZE - read_cnt(ldata) - 1;
+ left = N_TTY_BUF_SIZE - (head - tail) - 1;
Actually, less room may be available, if read_head != commit_head.
Could this cause problems? I guess yes, at least in
n_tty_receive_buf_common, where this could lead to a buffer overflow,
right?
Best regards,
Christian
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] n_tty: Fix unordered accesses to lockless read buffer
On Thu, Jan 1, 2015 at 12:00 PM, Christian Riesch @@ -164,15
+160,17 @@ static inline int tty_put_user(struct tty_struct *tty,
unsigned char x,
static int receive_room(struct tty_struct *tty)
{
struct n_tty_data *ldata = tty-disc_data;
+ size_t head = ACCESS_ONCE(ldata-commit_head);
+ size_t tail = ACCESS_ONCE(ldata-read_tail);
int left;
if (I_PARMRK(tty)) {
- /* Multiply read_cnt by 3, since each byte might take up to
+ /* Multiply count by 3, since each byte might take up to
* three times as many spaces when PARMRK is set (depending
on
* its flags, e.g. parity error). */
- left = N_TTY_BUF_SIZE - read_cnt(ldata) * 3 - 1;
+ left = N_TTY_BUF_SIZE - (head - tail) * 3 - 1;
} else
- left = N_TTY_BUF_SIZE - read_cnt(ldata) - 1;
+ left = N_TTY_BUF_SIZE - (head - tail) - 1;
Actually, less room may be available, if read_head != commit_head.
Could this cause problems? I guess yes, at least in
n_tty_receive_buf_common, where this could lead to a buffer overflow,
right?
Sorry, should not be a problem, at least not for
n_tty_receive_buf_common, since this is producer path, right? But how
about the other calls of receive_room()?
Christian
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH] n_tty: Add memory barriers where required for lock-less circular buffer
On Mon, Nov 17, 2014 at 1:27 PM, Christian Riesch
wrote:
> Signed-off-by: Christian Riesch
> Cc: Peter Hurley
[...]
> [2] https://lkml.org/lkml/2014/11/11/77
Uupps, [2] should point to https://lkml.org/lkml/2014/11/13/1 instead.
v3 instead of v2. Sorry.
>
> drivers/tty/n_tty.c | 93
> +++
> 1 file changed, 64 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
> index 47ca0f3..656d868 100644
> --- a/drivers/tty/n_tty.c
> +++ b/drivers/tty/n_tty.c
[...]
> @@ -165,14 +180,18 @@ static int receive_room(struct tty_struct *tty)
> {
> struct n_tty_data *ldata = tty->disc_data;
> int left;
> + size_t head, tail;
> +
> + head = ldata->read_head;
> + tail = ACCESS_ONCE(ldata->read_tail);
I just noticed that this is probably wrong, since receive_room is
called from both the consumer and producer paths.
>
> if (I_PARMRK(tty)) {
> - /* Multiply read_cnt by 3, since each byte might take up to
> -* three times as many spaces when PARMRK is set (depending on
> -* its flags, e.g. parity error). */
> - left = N_TTY_BUF_SIZE - read_cnt(ldata) * 3 - 1;
BTW does this make sense? When the data is in the buffer, it is
already expanded by the PARMRK feature. Only new data could be
expanded by a factor of 3 when PARMRK is set.
Shouldn't that be (N_TTY_BUF_SIZE - read_cnt(ldata) - 1)/3 ?
Regards, Christian
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[RFC PATCH] n_tty: Add memory barriers where required for lock-less circular buffer
Signed-off-by: Christian Riesch
Cc: Peter Hurley
---
Hi all,
This patch adds memory barriers to the lock-less circular buffer in the n_tty
driver to fix the race condition described in [1] and [2] for SMP machines.
The patch is based on the instructions in Documentation/circular-buffers.txt.
Currently, this is only compile-tested. Peter, could you please have a look
at this and the documentation mentioned above, and tell me what you
think about this?
Prerequisites:
- The patch in [2]
Thanks!
Christian
[1] https://lkml.org/lkml/2014/11/6/216
[2] https://lkml.org/lkml/2014/11/11/77
drivers/tty/n_tty.c | 93 +++
1 file changed, 64 insertions(+), 29 deletions(-)
diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
index 47ca0f3..656d868 100644
--- a/drivers/tty/n_tty.c
+++ b/drivers/tty/n_tty.c
@@ -127,9 +127,24 @@ struct n_tty_data {
struct mutex output_lock;
};
-static inline size_t read_cnt(struct n_tty_data *ldata)
+static inline size_t data_available(size_t head, size_t tail)
{
- return ldata->read_head - ldata->read_tail;
+ return head - tail;
+}
+
+static inline size_t room_available(size_t head, size_t tail)
+{
+ return N_TTY_BUF_SIZE - data_available(head, tail);
+}
+
+static inline size_t contiguous_data_available(size_t head, size_t tail)
+{
+ return min(head - tail, N_TTY_BUF_SIZE - (tail & (N_TTY_BUF_SIZE - 1)));
+}
+
+static inline size_t contiguous_room_available(size_t head, size_t tail)
+{
+ return N_TTY_BUF_SIZE - max(head - tail, head & (N_TTY_BUF_SIZE - 1));
}
static inline unsigned char read_buf(struct n_tty_data *ldata, size_t i)
@@ -165,14 +180,18 @@ static int receive_room(struct tty_struct *tty)
{
struct n_tty_data *ldata = tty->disc_data;
int left;
+ size_t head, tail;
+
+ head = ldata->read_head;
+ tail = ACCESS_ONCE(ldata->read_tail);
if (I_PARMRK(tty)) {
- /* Multiply read_cnt by 3, since each byte might take up to
-* three times as many spaces when PARMRK is set (depending on
-* its flags, e.g. parity error). */
- left = N_TTY_BUF_SIZE - read_cnt(ldata) * 3 - 1;
+ /* Multiply available data by 3, since each byte might take up
+* to three times as many spaces when PARMRK is set (depending
+* on its flags, e.g. parity error). */
+ left = N_TTY_BUF_SIZE - data_available(head, tail) * 3 - 1;
} else
- left = N_TTY_BUF_SIZE - read_cnt(ldata) - 1;
+ left = N_TTY_BUF_SIZE - data_available(head, tail) - 1;
/*
* If we are doing input canonicalization, and there are no
@@ -181,7 +200,7 @@ static int receive_room(struct tty_struct *tty)
* characters will be beeped.
*/
if (left <= 0)
- left = ldata->icanon && ldata->canon_head == ldata->read_tail;
+ left = ldata->icanon && ldata->canon_head == tail;
return left;
}
@@ -222,11 +241,15 @@ static ssize_t chars_in_buffer(struct tty_struct *tty)
{
struct n_tty_data *ldata = tty->disc_data;
ssize_t n = 0;
+ size_t head, tail;
+
+ head = ACCESS_ONCE(ldata->read_head);
+ tail = ldata->read_tail;
if (!ldata->icanon)
- n = read_cnt(ldata);
+ n = data_available(head, tail);
else
- n = ldata->canon_head - ldata->read_tail;
+ n = ldata->canon_head - tail;
return n;
}
@@ -322,7 +345,7 @@ static void n_tty_check_unthrottle(struct tty_struct *tty)
static inline void put_tty_queue(unsigned char c, struct n_tty_data *ldata)
{
*read_buf_addr(ldata, ldata->read_head) = c;
- ldata->read_head++;
+ smp_store_release(>read_head, ldata->read_head + 1);
}
/**
@@ -1534,21 +1557,23 @@ n_tty_receive_buf_real_raw(struct tty_struct *tty,
const unsigned char *cp,
char *fp, int count)
{
struct n_tty_data *ldata = tty->disc_data;
- size_t n, head;
+ size_t n, head, tail;
+
+ head = ldata->read_head;
+ tail = ACCESS_ONCE(ldata->read_tail);
- head = ldata->read_head & (N_TTY_BUF_SIZE - 1);
- n = N_TTY_BUF_SIZE - max(read_cnt(ldata), head);
+ n = contiguous_room_available(head, tail);
n = min_t(size_t, count, n);
memcpy(read_buf_addr(ldata, head), cp, n);
- ldata->read_head += n;
+ smp_store_release(>read_head, ldata->read_head + n);
cp += n;
count -= n;
- head = ldata->read_head & (N_TTY_BUF_SIZE - 1);
- n = N_TTY_BUF_SIZE - max(read_cnt(ldata), head);
+ head = ldata->read_head;
+ n = contiguous_room_available(head, tail);
n = min_t(size_t, count, n);
memcpy(
[RFC PATCH] n_tty: Add memory barriers where required for lock-less circular buffer
Signed-off-by: Christian Riesch christian.rie...@omicron.at
Cc: Peter Hurley pe...@hurleysoftware.com
---
Hi all,
This patch adds memory barriers to the lock-less circular buffer in the n_tty
driver to fix the race condition described in [1] and [2] for SMP machines.
The patch is based on the instructions in Documentation/circular-buffers.txt.
Currently, this is only compile-tested. Peter, could you please have a look
at this and the documentation mentioned above, and tell me what you
think about this?
Prerequisites:
- The patch in [2]
Thanks!
Christian
[1] https://lkml.org/lkml/2014/11/6/216
[2] https://lkml.org/lkml/2014/11/11/77
drivers/tty/n_tty.c | 93 +++
1 file changed, 64 insertions(+), 29 deletions(-)
diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
index 47ca0f3..656d868 100644
--- a/drivers/tty/n_tty.c
+++ b/drivers/tty/n_tty.c
@@ -127,9 +127,24 @@ struct n_tty_data {
struct mutex output_lock;
};
-static inline size_t read_cnt(struct n_tty_data *ldata)
+static inline size_t data_available(size_t head, size_t tail)
{
- return ldata-read_head - ldata-read_tail;
+ return head - tail;
+}
+
+static inline size_t room_available(size_t head, size_t tail)
+{
+ return N_TTY_BUF_SIZE - data_available(head, tail);
+}
+
+static inline size_t contiguous_data_available(size_t head, size_t tail)
+{
+ return min(head - tail, N_TTY_BUF_SIZE - (tail (N_TTY_BUF_SIZE - 1)));
+}
+
+static inline size_t contiguous_room_available(size_t head, size_t tail)
+{
+ return N_TTY_BUF_SIZE - max(head - tail, head (N_TTY_BUF_SIZE - 1));
}
static inline unsigned char read_buf(struct n_tty_data *ldata, size_t i)
@@ -165,14 +180,18 @@ static int receive_room(struct tty_struct *tty)
{
struct n_tty_data *ldata = tty-disc_data;
int left;
+ size_t head, tail;
+
+ head = ldata-read_head;
+ tail = ACCESS_ONCE(ldata-read_tail);
if (I_PARMRK(tty)) {
- /* Multiply read_cnt by 3, since each byte might take up to
-* three times as many spaces when PARMRK is set (depending on
-* its flags, e.g. parity error). */
- left = N_TTY_BUF_SIZE - read_cnt(ldata) * 3 - 1;
+ /* Multiply available data by 3, since each byte might take up
+* to three times as many spaces when PARMRK is set (depending
+* on its flags, e.g. parity error). */
+ left = N_TTY_BUF_SIZE - data_available(head, tail) * 3 - 1;
} else
- left = N_TTY_BUF_SIZE - read_cnt(ldata) - 1;
+ left = N_TTY_BUF_SIZE - data_available(head, tail) - 1;
/*
* If we are doing input canonicalization, and there are no
@@ -181,7 +200,7 @@ static int receive_room(struct tty_struct *tty)
* characters will be beeped.
*/
if (left = 0)
- left = ldata-icanon ldata-canon_head == ldata-read_tail;
+ left = ldata-icanon ldata-canon_head == tail;
return left;
}
@@ -222,11 +241,15 @@ static ssize_t chars_in_buffer(struct tty_struct *tty)
{
struct n_tty_data *ldata = tty-disc_data;
ssize_t n = 0;
+ size_t head, tail;
+
+ head = ACCESS_ONCE(ldata-read_head);
+ tail = ldata-read_tail;
if (!ldata-icanon)
- n = read_cnt(ldata);
+ n = data_available(head, tail);
else
- n = ldata-canon_head - ldata-read_tail;
+ n = ldata-canon_head - tail;
return n;
}
@@ -322,7 +345,7 @@ static void n_tty_check_unthrottle(struct tty_struct *tty)
static inline void put_tty_queue(unsigned char c, struct n_tty_data *ldata)
{
*read_buf_addr(ldata, ldata-read_head) = c;
- ldata-read_head++;
+ smp_store_release(ldata-read_head, ldata-read_head + 1);
}
/**
@@ -1534,21 +1557,23 @@ n_tty_receive_buf_real_raw(struct tty_struct *tty,
const unsigned char *cp,
char *fp, int count)
{
struct n_tty_data *ldata = tty-disc_data;
- size_t n, head;
+ size_t n, head, tail;
+
+ head = ldata-read_head;
+ tail = ACCESS_ONCE(ldata-read_tail);
- head = ldata-read_head (N_TTY_BUF_SIZE - 1);
- n = N_TTY_BUF_SIZE - max(read_cnt(ldata), head);
+ n = contiguous_room_available(head, tail);
n = min_t(size_t, count, n);
memcpy(read_buf_addr(ldata, head), cp, n);
- ldata-read_head += n;
+ smp_store_release(ldata-read_head, ldata-read_head + n);
cp += n;
count -= n;
- head = ldata-read_head (N_TTY_BUF_SIZE - 1);
- n = N_TTY_BUF_SIZE - max(read_cnt(ldata), head);
+ head = ldata-read_head;
+ n = contiguous_room_available(head, tail);
n = min_t(size_t, count, n);
memcpy(read_buf_addr(ldata, head), cp, n);
- ldata-read_head += n;
+ smp_store_release(ldata
Re: [RFC PATCH] n_tty: Add memory barriers where required for lock-less circular buffer
On Mon, Nov 17, 2014 at 1:27 PM, Christian Riesch
christian.rie...@omicron.at wrote:
Signed-off-by: Christian Riesch christian.rie...@omicron.at
Cc: Peter Hurley pe...@hurleysoftware.com
[...]
[2] https://lkml.org/lkml/2014/11/11/77
Uupps, [2] should point to https://lkml.org/lkml/2014/11/13/1 instead.
v3 instead of v2. Sorry.
drivers/tty/n_tty.c | 93
+++
1 file changed, 64 insertions(+), 29 deletions(-)
diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
index 47ca0f3..656d868 100644
--- a/drivers/tty/n_tty.c
+++ b/drivers/tty/n_tty.c
[...]
@@ -165,14 +180,18 @@ static int receive_room(struct tty_struct *tty)
{
struct n_tty_data *ldata = tty-disc_data;
int left;
+ size_t head, tail;
+
+ head = ldata-read_head;
+ tail = ACCESS_ONCE(ldata-read_tail);
I just noticed that this is probably wrong, since receive_room is
called from both the consumer and producer paths.
if (I_PARMRK(tty)) {
- /* Multiply read_cnt by 3, since each byte might take up to
-* three times as many spaces when PARMRK is set (depending on
-* its flags, e.g. parity error). */
- left = N_TTY_BUF_SIZE - read_cnt(ldata) * 3 - 1;
BTW does this make sense? When the data is in the buffer, it is
already expanded by the PARMRK feature. Only new data could be
expanded by a factor of 3 when PARMRK is set.
Shouldn't that be (N_TTY_BUF_SIZE - read_cnt(ldata) - 1)/3 ?
Regards, Christian
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH v3] n_tty: Fix read_buf race condition, increment read_head after pushing data
Commit 19e2ad6a09f0c06dbca19c98e5f4584269d913dd ("n_tty: Remove overflow
tests from receive_buf() path") moved the increment of read_head into
the arguments list of read_buf_addr(). Function calls represent a
sequence point in C. Therefore read_head is incremented before the
character c is placed in the buffer. Since the circular read buffer is
a lock-less design since commit 6d76bd2618535c581f1673047b8341fd291abc67
("n_tty: Make N_TTY ldisc receive path lockless"), this creates a race
condition that leads to communication errors.
This patch modifies the code to increment read_head _after_ the data
is placed in the buffer and thus fixes the race for non-SMP machines.
To fix the problem for SMP machines, memory barriers must be added in
a separate patch.
Signed-off-by: Christian Riesch
Cc:
---
Changes for v3:
- Removed unnecessary comment.
Changes for v2:
- Rewrote commit message. Since I did not know better, I blamed the compiler
in v1, but actually the code was wrong. See the discussion in [1].
- Removed memory barriers. For non-SMP machines they are not required,
for SMP machines more brainwork and discussions are needed.
drivers/tty/n_tty.c |3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
index 2e900a9..47ca0f3 100644
--- a/drivers/tty/n_tty.c
+++ b/drivers/tty/n_tty.c
@@ -321,7 +321,8 @@ static void n_tty_check_unthrottle(struct tty_struct *tty)
static inline void put_tty_queue(unsigned char c, struct n_tty_data *ldata)
{
- *read_buf_addr(ldata, ldata->read_head++) = c;
+ *read_buf_addr(ldata, ldata->read_head) = c;
+ ldata->read_head++;
}
/**
--
1.7.9.5
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] n_tty: Fix read_buf race condition, increment read_head after pushing data
On Wed, Nov 12, 2014 at 12:53 PM, Måns Rullgård wrote:
> Christian Riesch writes:
>
>> On Tue, Nov 11, 2014 at 2:04 PM, Måns Rullgård wrote:
>>> Christian Riesch writes:
>> [...]>> diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
>>>> index 2e900a9..b09f326 100644
>>>> --- a/drivers/tty/n_tty.c
>>>> +++ b/drivers/tty/n_tty.c
>>>> @@ -321,7 +321,9 @@ static void n_tty_check_unthrottle(struct tty_struct
>>>> *tty)
>>>>
>>>> static inline void put_tty_queue(unsigned char c, struct n_tty_data
>>>> *ldata)
>>>> {
>>>> - *read_buf_addr(ldata, ldata->read_head++) = c;
>>>> + *read_buf_addr(ldata, ldata->read_head) = c;
>>>> + /* increment read_head _after_ placing the character in the buffer */
>>>> + ldata->read_head++;
>>>> }
>>>
>>> Is that comment really necessary?
>>
>> No, I am pretty sure that removing the comment would not break the code ;-)
>>
>> I just thought it would be good to have some kind of reminder here.
>> Otherwise someone may think: Hey, it would be a good idea to do the
>> increment right in the first line. And submit a patch for it.
>
> The intent all along was to increment after the write. Nobody needs
> reminding of that. The problem was a misunderstanding of when the
> post-increment takes effect. As much as we'd like for everybody to have
> a thorough knowledge of C, a random tty driver doesn't seem the place to
> educate them.
Ok. I will send a new patch without the comment.
Thanks, Christian
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] n_tty: Fix read_buf race condition, increment read_head after pushing data
On Wed, Nov 12, 2014 at 12:53 PM, Måns Rullgård m...@mansr.com wrote:
Christian Riesch christian.rie...@omicron.at writes:
On Tue, Nov 11, 2014 at 2:04 PM, Måns Rullgård m...@mansr.com wrote:
Christian Riesch christian.rie...@omicron.at writes:
[...] diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
index 2e900a9..b09f326 100644
--- a/drivers/tty/n_tty.c
+++ b/drivers/tty/n_tty.c
@@ -321,7 +321,9 @@ static void n_tty_check_unthrottle(struct tty_struct
*tty)
static inline void put_tty_queue(unsigned char c, struct n_tty_data
*ldata)
{
- *read_buf_addr(ldata, ldata-read_head++) = c;
+ *read_buf_addr(ldata, ldata-read_head) = c;
+ /* increment read_head _after_ placing the character in the buffer */
+ ldata-read_head++;
}
Is that comment really necessary?
No, I am pretty sure that removing the comment would not break the code ;-)
I just thought it would be good to have some kind of reminder here.
Otherwise someone may think: Hey, it would be a good idea to do the
increment right in the first line. And submit a patch for it.
The intent all along was to increment after the write. Nobody needs
reminding of that. The problem was a misunderstanding of when the
post-increment takes effect. As much as we'd like for everybody to have
a thorough knowledge of C, a random tty driver doesn't seem the place to
educate them.
Ok. I will send a new patch without the comment.
Thanks, Christian
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH v3] n_tty: Fix read_buf race condition, increment read_head after pushing data
Commit 19e2ad6a09f0c06dbca19c98e5f4584269d913dd (n_tty: Remove overflow
tests from receive_buf() path) moved the increment of read_head into
the arguments list of read_buf_addr(). Function calls represent a
sequence point in C. Therefore read_head is incremented before the
character c is placed in the buffer. Since the circular read buffer is
a lock-less design since commit 6d76bd2618535c581f1673047b8341fd291abc67
(n_tty: Make N_TTY ldisc receive path lockless), this creates a race
condition that leads to communication errors.
This patch modifies the code to increment read_head _after_ the data
is placed in the buffer and thus fixes the race for non-SMP machines.
To fix the problem for SMP machines, memory barriers must be added in
a separate patch.
Signed-off-by: Christian Riesch christian.rie...@omicron.at
Cc: sta...@vger.kernel.org
---
Changes for v3:
- Removed unnecessary comment.
Changes for v2:
- Rewrote commit message. Since I did not know better, I blamed the compiler
in v1, but actually the code was wrong. See the discussion in [1].
- Removed memory barriers. For non-SMP machines they are not required,
for SMP machines more brainwork and discussions are needed.
drivers/tty/n_tty.c |3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
index 2e900a9..47ca0f3 100644
--- a/drivers/tty/n_tty.c
+++ b/drivers/tty/n_tty.c
@@ -321,7 +321,8 @@ static void n_tty_check_unthrottle(struct tty_struct *tty)
static inline void put_tty_queue(unsigned char c, struct n_tty_data *ldata)
{
- *read_buf_addr(ldata, ldata-read_head++) = c;
+ *read_buf_addr(ldata, ldata-read_head) = c;
+ ldata-read_head++;
}
/**
--
1.7.9.5
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] n_tty: Fix read_buf race condition, increment read_head after pushing data
On Tue, Nov 11, 2014 at 2:04 PM, Måns Rullgård wrote:
> Christian Riesch writes:
[...]>> diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
>> index 2e900a9..b09f326 100644
>> --- a/drivers/tty/n_tty.c
>> +++ b/drivers/tty/n_tty.c
>> @@ -321,7 +321,9 @@ static void n_tty_check_unthrottle(struct tty_struct
>> *tty)
>>
>> static inline void put_tty_queue(unsigned char c, struct n_tty_data *ldata)
>> {
>> - *read_buf_addr(ldata, ldata->read_head++) = c;
>> + *read_buf_addr(ldata, ldata->read_head) = c;
>> + /* increment read_head _after_ placing the character in the buffer */
>> + ldata->read_head++;
>> }
>
> Is that comment really necessary?
No, I am pretty sure that removing the comment would not break the code ;-)
I just thought it would be good to have some kind of reminder here.
Otherwise someone may think: Hey, it would be a good idea to do the
increment right in the first line. And submit a patch for it.
But I am also ok with removing the comment. So if you like me to post
a v3 without the comment, I'll be happy to do that.
Christian
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] n_tty: Fix read_buf race condition, increment read_head after pushing data
On Tue, Nov 11, 2014 at 2:04 PM, Måns Rullgård m...@mansr.com wrote:
Christian Riesch christian.rie...@omicron.at writes:
[...] diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
index 2e900a9..b09f326 100644
--- a/drivers/tty/n_tty.c
+++ b/drivers/tty/n_tty.c
@@ -321,7 +321,9 @@ static void n_tty_check_unthrottle(struct tty_struct
*tty)
static inline void put_tty_queue(unsigned char c, struct n_tty_data *ldata)
{
- *read_buf_addr(ldata, ldata-read_head++) = c;
+ *read_buf_addr(ldata, ldata-read_head) = c;
+ /* increment read_head _after_ placing the character in the buffer */
+ ldata-read_head++;
}
Is that comment really necessary?
No, I am pretty sure that removing the comment would not break the code ;-)
I just thought it would be good to have some kind of reminder here.
Otherwise someone may think: Hey, it would be a good idea to do the
increment right in the first line. And submit a patch for it.
But I am also ok with removing the comment. So if you like me to post
a v3 without the comment, I'll be happy to do that.
Christian
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH v2] n_tty: Fix read_buf race condition, increment read_head after pushing data
Commit 19e2ad6a09f0c06dbca19c98e5f4584269d913dd ("n_tty: Remove overflow
tests from receive_buf() path") moved the increment of read_head into
the arguments list of read_buf_addr(). Function calls represent a
sequence point in C. Therefore read_head is incremented before the
character c is placed in the buffer. Since the circular read buffer is
a lock-less design since commit 6d76bd2618535c581f1673047b8341fd291abc67
("n_tty: Make N_TTY ldisc receive path lockless"), this creates a race
condition that leads to communication errors.
This patch modifies the code to increment read_head _after_ the data
is placed in the buffer and thus fixes the race for non-SMP machines.
To fix the problem for SMP machines, memory barriers must be added in
a separate patch.
Signed-off-by: Christian Riesch
Cc:
---
This is version 2 of the patch in [1].
Changes for v2:
- Rewrote commit message. Since I did not know better, I blamed the compiler
in v1, but actually the code was wrong. See the discussion in [1].
- Removed memory barriers. For non-SMP machines they are not required,
for SMP machines more brainwork and discussions are needed.
Best regards,
Christian
[1] https://lkml.org/lkml/2014/11/6/216
drivers/tty/n_tty.c |4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
index 2e900a9..b09f326 100644
--- a/drivers/tty/n_tty.c
+++ b/drivers/tty/n_tty.c
@@ -321,7 +321,9 @@ static void n_tty_check_unthrottle(struct tty_struct *tty)
static inline void put_tty_queue(unsigned char c, struct n_tty_data *ldata)
{
- *read_buf_addr(ldata, ldata->read_head++) = c;
+ *read_buf_addr(ldata, ldata->read_head) = c;
+ /* increment read_head _after_ placing the character in the buffer */
+ ldata->read_head++;
}
/**
--
1.7.9.5
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] n_tty: Add memory barrier to fix race condition in receive path
Hi Måns, On Mon, Nov 10, 2014 at 10:25 AM, Måns Rullgård wrote: > Christian Riesch writes: > >> On Thu, Nov 6, 2014 at 9:56 PM, Greg Kroah-Hartman >> wrote: >>> On Thu, Nov 06, 2014 at 08:49:01PM +, Måns Rullgård wrote: >>>> Greg Kroah-Hartman writes: >>>> >>>> > On Thu, Nov 06, 2014 at 12:39:59PM +0100, Christian Riesch wrote: >>>> >> The current implementation of put_tty_queue() causes a race condition >>>> >> when re-arranged by the compiler. >>>> >> >>>> >> On my build with gcc 4.8.3, cross-compiling for ARM, the line >>>> >> >>>> >>*read_buf_addr(ldata, ldata->read_head++) = c; >>>> >> >>>> >> was re-arranged by the compiler to something like >>>> >> >>>> >>x = ldata->read_head >>>> >>ldata->read_head++ >>>> >>*read_buf_addr(ldata, x) = c; >>>> >> >>>> >> which causes a race condition. Invalid data is read if data is read >>>> >> before it is actually written to the read buffer. >>>> > >>>> > Really? A compiler can rearange things like that and expect things to >>>> > actually work? How is that valid? >>>> >>>> This is actually required by the C spec. There is a sequence point >>>> before a function call, after the arguments have been evaluated. Thus >>>> all side-effects, such as the post-increment, must be complete before >>>> the function is called, just like in the example. >>>> >>>> There is no "re-arranging" here. The code is simply wrong. >>> >>> Ah, ok, time to dig out the C spec... >>> >>> Anyway, because of this, no need for the wmb() calls, just rearrange the >>> logic and all should be good, right? Christian, can you test that >>> instead? >> >> I ran a test with the patch that I posted in my first email for the >> last 4 days. No communication errors occurred so the patch actually >> fixes my problem. I will run another test as suggested by Greg, just >> with rearranging the logic. > > What hardware are you running on? If it's a single-processor system, > it won't break without barriers even if they are required for SMP. Yes, single processor. Texas Instruments AM1808 SoC. Thanks, Christian -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] n_tty: Add memory barrier to fix race condition in receive path
Hi Måns, On Mon, Nov 10, 2014 at 10:25 AM, Måns Rullgård m...@mansr.com wrote: Christian Riesch christian.rie...@omicron.at writes: On Thu, Nov 6, 2014 at 9:56 PM, Greg Kroah-Hartman gre...@linuxfoundation.org wrote: On Thu, Nov 06, 2014 at 08:49:01PM +, Måns Rullgård wrote: Greg Kroah-Hartman gre...@linuxfoundation.org writes: On Thu, Nov 06, 2014 at 12:39:59PM +0100, Christian Riesch wrote: The current implementation of put_tty_queue() causes a race condition when re-arranged by the compiler. On my build with gcc 4.8.3, cross-compiling for ARM, the line *read_buf_addr(ldata, ldata-read_head++) = c; was re-arranged by the compiler to something like x = ldata-read_head ldata-read_head++ *read_buf_addr(ldata, x) = c; which causes a race condition. Invalid data is read if data is read before it is actually written to the read buffer. Really? A compiler can rearange things like that and expect things to actually work? How is that valid? This is actually required by the C spec. There is a sequence point before a function call, after the arguments have been evaluated. Thus all side-effects, such as the post-increment, must be complete before the function is called, just like in the example. There is no re-arranging here. The code is simply wrong. Ah, ok, time to dig out the C spec... Anyway, because of this, no need for the wmb() calls, just rearrange the logic and all should be good, right? Christian, can you test that instead? I ran a test with the patch that I posted in my first email for the last 4 days. No communication errors occurred so the patch actually fixes my problem. I will run another test as suggested by Greg, just with rearranging the logic. What hardware are you running on? If it's a single-processor system, it won't break without barriers even if they are required for SMP. Yes, single processor. Texas Instruments AM1808 SoC. Thanks, Christian -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH v2] n_tty: Fix read_buf race condition, increment read_head after pushing data
Commit 19e2ad6a09f0c06dbca19c98e5f4584269d913dd (n_tty: Remove overflow
tests from receive_buf() path) moved the increment of read_head into
the arguments list of read_buf_addr(). Function calls represent a
sequence point in C. Therefore read_head is incremented before the
character c is placed in the buffer. Since the circular read buffer is
a lock-less design since commit 6d76bd2618535c581f1673047b8341fd291abc67
(n_tty: Make N_TTY ldisc receive path lockless), this creates a race
condition that leads to communication errors.
This patch modifies the code to increment read_head _after_ the data
is placed in the buffer and thus fixes the race for non-SMP machines.
To fix the problem for SMP machines, memory barriers must be added in
a separate patch.
Signed-off-by: Christian Riesch christian.rie...@omicron.at
Cc: sta...@vger.kernel.org
---
This is version 2 of the patch in [1].
Changes for v2:
- Rewrote commit message. Since I did not know better, I blamed the compiler
in v1, but actually the code was wrong. See the discussion in [1].
- Removed memory barriers. For non-SMP machines they are not required,
for SMP machines more brainwork and discussions are needed.
Best regards,
Christian
[1] https://lkml.org/lkml/2014/11/6/216
drivers/tty/n_tty.c |4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
index 2e900a9..b09f326 100644
--- a/drivers/tty/n_tty.c
+++ b/drivers/tty/n_tty.c
@@ -321,7 +321,9 @@ static void n_tty_check_unthrottle(struct tty_struct *tty)
static inline void put_tty_queue(unsigned char c, struct n_tty_data *ldata)
{
- *read_buf_addr(ldata, ldata-read_head++) = c;
+ *read_buf_addr(ldata, ldata-read_head) = c;
+ /* increment read_head _after_ placing the character in the buffer */
+ ldata-read_head++;
}
/**
--
1.7.9.5
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] n_tty: Add memory barrier to fix race condition in receive path
On Thu, Nov 6, 2014 at 9:56 PM, Greg Kroah-Hartman wrote: > On Thu, Nov 06, 2014 at 08:49:01PM +, Måns Rullgård wrote: >> Greg Kroah-Hartman writes: >> >> > On Thu, Nov 06, 2014 at 12:39:59PM +0100, Christian Riesch wrote: >> >> The current implementation of put_tty_queue() causes a race condition >> >> when re-arranged by the compiler. >> >> >> >> On my build with gcc 4.8.3, cross-compiling for ARM, the line >> >> >> >>*read_buf_addr(ldata, ldata->read_head++) = c; >> >> >> >> was re-arranged by the compiler to something like >> >> >> >>x = ldata->read_head >> >>ldata->read_head++ >> >>*read_buf_addr(ldata, x) = c; >> >> >> >> which causes a race condition. Invalid data is read if data is read >> >> before it is actually written to the read buffer. >> > >> > Really? A compiler can rearange things like that and expect things to >> > actually work? How is that valid? >> >> This is actually required by the C spec. There is a sequence point >> before a function call, after the arguments have been evaluated. Thus >> all side-effects, such as the post-increment, must be complete before >> the function is called, just like in the example. >> >> There is no "re-arranging" here. The code is simply wrong. > > Ah, ok, time to dig out the C spec... > > Anyway, because of this, no need for the wmb() calls, just rearrange the > logic and all should be good, right? Christian, can you test that > instead? I ran a test with the patch that I posted in my first email for the last 4 days. No communication errors occurred so the patch actually fixes my problem. I will run another test as suggested by Greg, just with rearranging the logic. Best regards, Christian -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] n_tty: Add memory barrier to fix race condition in receive path
On Thu, Nov 6, 2014 at 9:56 PM, Greg Kroah-Hartman gre...@linuxfoundation.org wrote: On Thu, Nov 06, 2014 at 08:49:01PM +, Måns Rullgård wrote: Greg Kroah-Hartman gre...@linuxfoundation.org writes: On Thu, Nov 06, 2014 at 12:39:59PM +0100, Christian Riesch wrote: The current implementation of put_tty_queue() causes a race condition when re-arranged by the compiler. On my build with gcc 4.8.3, cross-compiling for ARM, the line *read_buf_addr(ldata, ldata-read_head++) = c; was re-arranged by the compiler to something like x = ldata-read_head ldata-read_head++ *read_buf_addr(ldata, x) = c; which causes a race condition. Invalid data is read if data is read before it is actually written to the read buffer. Really? A compiler can rearange things like that and expect things to actually work? How is that valid? This is actually required by the C spec. There is a sequence point before a function call, after the arguments have been evaluated. Thus all side-effects, such as the post-increment, must be complete before the function is called, just like in the example. There is no re-arranging here. The code is simply wrong. Ah, ok, time to dig out the C spec... Anyway, because of this, no need for the wmb() calls, just rearrange the logic and all should be good, right? Christian, can you test that instead? I ran a test with the patch that I posted in my first email for the last 4 days. No communication errors occurred so the patch actually fixes my problem. I will run another test as suggested by Greg, just with rearranging the logic. Best regards, Christian -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] n_tty: Add memory barrier to fix race condition in receive path
[sent again due to stupid HTML mail problems, sorry]
On Thu, Nov 6, 2014 at 11:54 PM, Måns Rullgård wrote:
> Greg Kroah-Hartman writes:
>
>> On Thu, Nov 06, 2014 at 10:12:54PM +, Måns Rullgård wrote:
>>> Greg Kroah-Hartman writes:
>>>
>>> > On Thu, Nov 06, 2014 at 09:38:59PM +, Måns Rullgård wrote:
>>> >> Greg Kroah-Hartman writes:
>>> >>
>>> >> > On Thu, Nov 06, 2014 at 09:01:36PM +, Måns Rullgård wrote:
>>> >> >> Greg Kroah-Hartman writes:
>>> >> >>
>>> >> >> > On Thu, Nov 06, 2014 at 08:49:01PM +, Måns Rullgård wrote:
>>> >> >> >> Greg Kroah-Hartman writes:
>>> >> >> >>
>>> >> >> >> > On Thu, Nov 06, 2014 at 12:39:59PM +0100, Christian Riesch wrote:
>>> >> >> >> >> The current implementation of put_tty_queue() causes a race
>>> >> >> >> >> condition
>>> >> >> >> >> when re-arranged by the compiler.
>>> >> >> >> >>
>>> >> >> >> >> On my build with gcc 4.8.3, cross-compiling for ARM, the line
>>> >> >> >> >>
>>> >> >> >> >> *read_buf_addr(ldata, ldata->read_head++) = c;
>>> >> >> >> >>
>>> >> >> >> >> was re-arranged by the compiler to something like
>>> >> >> >> >>
>>> >> >> >> >> x = ldata->read_head
>>> >> >> >> >> ldata->read_head++
>>> >> >> >> >> *read_buf_addr(ldata, x) = c;
>>> >> >> >> >>
>>> >> >> >> >> which causes a race condition. Invalid data is read if data is
>>> >> >> >> >> read
>>> >> >> >> >> before it is actually written to the read buffer.
>>> >> >> >> >
>>> >> >> >> > Really? A compiler can rearange things like that and expect
>>> >> >> >> > things to
>>> >> >> >> > actually work? How is that valid?
>>> >> >> >>
>>> >> >> >> This is actually required by the C spec. There is a sequence point
>>> >> >> >> before a function call, after the arguments have been evaluated.
>>> >> >> >> Thus
>>> >> >> >> all side-effects, such as the post-increment, must be complete
>>> >> >> >> before
>>> >> >> >> the function is called, just like in the example.
>>> >> >> >>
>>> >> >> >> There is no "re-arranging" here. The code is simply wrong.
>>> >> >> >
>>> >> >> > Ah, ok, time to dig out the C spec...
>>> >> >> >
>>> >> >> > Anyway, because of this, no need for the wmb() calls, just
>>> >> >> > rearrange the
>>> >> >> > logic and all should be good, right? Christian, can you test that
>>> >> >> > instead?
>>> >> >>
>>> >> >> Weakly ordered SMP systems probably need some kind of barrier. I
>>> >> >> didn't
>>> >> >> look at it carefully.
>>> >> >
>>> >> > It shouldn't need a barier, as it is a sequence point with the function
>>> >> > call. Well, it's an inline function, but that "shouldn't" matter here,
>>> >> > right?
>>> >>
>>> >> Sequence points say nothing about the order in which stores become
>>> >> visible to other CPUs. That's why there are barrier instructions.
>>> >
>>> > Yes, but "order" matters.
>>> >
>>> > If I write code that does:
>>> >
>>> > 100x = ldata->read_head;
>>> > 101>read_head[x & SOME_VALUE] = y;
>>> > 102ldata->read_head++;
>>> >
>>> > the compiler can not reorder lines 102 and 101 just because it feels
>>> > like it, right? Or is it time to go spend some reading of the C spec
>>> > again...
>>>
>>> The compiler can't. The hardware can. All the hardware promises is
>>> that at some unspecified time in the future, both memory locations will
>>> have the correct values. Another CPU might see 'read_head' updated
>>> before it sees the corresponding data value. A wmb() between the writes
>>> forces the CPU to complete preceding stores before it begins subsequent
>>> ones.
>>
>> Yes, sorry, I'm not talking about other CPUs and what they see, I'm
>> talking about the local one. I'm not assuming that this is SMP "safe"
>> at all. If it is supposed to be, then yes, we do have problems, but
>> there should be a lock _somewhere_ protecting this.
>
> Within the confines of a single CPU + memory, barriers are never needed.
> The moment another CPU or master-capable peripheral enters the mix,
> proper ordering must be enforced somehow.
>
> If the buffer is already protected by a lock of some kind, this will
> provide the necessary barriers, so nothing further is necessary. If
> it's a lock-less design, there will need to be barriers somewhere.
It was changed to lock-less with 3.12 in commit
6d76bd2618535c581f1673047b8341fd291abc67 ("n_tty: Make N_TTY ldisc
receive
path lockless"). So I will try to read the memory barrier docs again.
Of course my little ARM system is no SMP system, but I guess this
should also be fixed for the SMP case, right?
Thanks,
Christian
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] n_tty: Add memory barrier to fix race condition in receive path
On Thu, Nov 6, 2014 at 9:56 PM, Greg Kroah-Hartman wrote: > On Thu, Nov 06, 2014 at 08:49:01PM +, Måns Rullgård wrote: >> Greg Kroah-Hartman writes: >> >> > On Thu, Nov 06, 2014 at 12:39:59PM +0100, Christian Riesch wrote: >> >> The current implementation of put_tty_queue() causes a race condition >> >> when re-arranged by the compiler. >> >> >> >> On my build with gcc 4.8.3, cross-compiling for ARM, the line >> >> >> >>*read_buf_addr(ldata, ldata->read_head++) = c; >> >> >> >> was re-arranged by the compiler to something like >> >> >> >>x = ldata->read_head >> >>ldata->read_head++ >> >>*read_buf_addr(ldata, x) = c; >> >> >> >> which causes a race condition. Invalid data is read if data is read >> >> before it is actually written to the read buffer. >> > >> > Really? A compiler can rearange things like that and expect things to >> > actually work? How is that valid? >> >> This is actually required by the C spec. There is a sequence point >> before a function call, after the arguments have been evaluated. Thus >> all side-effects, such as the post-increment, must be complete before >> the function is called, just like in the example. >> >> There is no "re-arranging" here. The code is simply wrong. Oh, I didn't know that, thanks a lot for this! > Anyway, because of this, no need for the wmb() calls, just rearrange the > logic and all should be good, right? I came up with the wmb() stuff after getting scared from reading Documentation/memory-barriers.txt (which I didn't understand) and Documentation/circular-buffers.txt (which I understood partly). But it is actually a circular buffer and circular-buffers.txt says I need memory barriers for circular buffers. Though I do not know how function calls fit into this picture. > Christian, can you test that > instead? Sure, but it will probably not happen before Monday. Thanks a lot for your help! Christian -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] n_tty: Add memory barrier to fix race condition in receive path
The current implementation of put_tty_queue() causes a race condition
when re-arranged by the compiler.
On my build with gcc 4.8.3, cross-compiling for ARM, the line
*read_buf_addr(ldata, ldata->read_head++) = c;
was re-arranged by the compiler to something like
x = ldata->read_head
ldata->read_head++
*read_buf_addr(ldata, x) = c;
which causes a race condition. Invalid data is read if data is read
before it is actually written to the read buffer.
This race condition was introduced with commit
6d76bd2618535c581f1673047b8341fd291abc67 ("n_tty: Make N_TTY ldisc receive
path lockless").
This patch adds memory barriers to resolve this race condition.
Signed-off-by: Christian Riesch
Cc: Peter Hurley
Cc:
---
Hi all,
I noticed that since an upgrade to kernel 3.12 my ARM device communicating
via serial port with a GPS receiver module reports frequent communication
errors.
After several attempts to resolve these problems I created a small
test setup. This setup utilizes a small microcontroller that just sends
data via serial port to the ARM processor using 9600o1.
The code on the microcontroller looks like this:
char c = 48;
while (1) {
if c > 126 then c = 48
send character c
c++
}
On the ARM/Linux side I ran the serial port in non-canonical mode,
received the data and checked if the data is what we expect it to be:
struct termios tio;
memset(, 0, sizeof(tio));
tio.c_cflag = CREAD | CLOCAL | B9600 | CS8 | PARENB | PARODD;
tio.c_iflag = INPCK;
tio.c_cc[VTIME] = 0;
tio.c_cc[VMIN] = 0;
tcsetattr(fd, TCSANOW, );
...
c = 48;
while (1) {
...
poll(pfds, 1, 1000);
if (pfds[0].revents & POLLIN) {
ret = read(fd, buf, 200);
for (i = 0; i < ret; i++) {
c++;
if (c > 126)
c = 48;
if (c != buf[i]) {
printf("expected %d - received %d, ret = %d, i = %d\n",
c, buf[i], ret, i);
c = buf[i];
}
}
}
}
I ran this test for about 5 days, the result was:
expected 51 - received 63, ret = 11, i = 10
expected 64 - received 52, ret = 13, i = 0
expected 64 - received 76, ret = 11, i = 10
expected 77 - received 65, ret = 5, i = 0
expected 105 - received 117, ret = 18, i = 17
expected 118 - received 106, ret = 6, i = 0
expected 120 - received 53, ret = 16, i = 15
expected 54 - received 121, ret = 8, i = 0
expected 105 - received 117, ret = 3, i = 2
expected 118 - received 106, ret = 5, i = 0
expected 79 - received 91, ret = 20, i = 19
expected 92 - received 80, ret = 4, i = 0
expected 72 - received 84, ret = 15, i = 14
expected 85 - received 73, ret = 9, i = 0
expected 54 - received 66, ret = 13, i = 12
expected 67 - received 55, ret = 3, i = 0
expected 86 - received 98, ret = 25, i = 24
expected 99 - received 87, ret = 15, i = 0
expected 86 - received 98, ret = 14, i = 13
expected 99 - received 87, ret = 42, i = 0
expected 93 - received 105, ret = 34, i = 33
expected 106 - received 94, ret = 6, i = 0
expected 92 - received 104, ret = 16, i = 15
expected 105 - received 93, ret = 8, i = 0
expected 53 - received 65, ret = 8, i = 7
expected 66 - received 54, ret = 8, i = 0
The first line shows that we expected buf[i] to be 51, actually we
received 63. We therefore set c = 63, consequently we expect 64 as the
next character. But we receive 52, so everything is back to normal. So
no bytes are missing, no additional bytes are received, there is just
a single byte with a wrong content.
We see that always the last byte in buf is affected, i.e. buf[ret - 1].
Furthermore we see that the wrong byte is always off by 12, i.e. instead
of 51 we received 63 (63 - 51 = 12), instead of 64 we received 76
(76 - 64 = 12) etc.
The race that I described above in the commit message exactly results in
such a behavior.
In the example below read_head was already incremented but the new content
has not yet been written to ldata->read_buf.
48 49 50 51 64 65 66 67
^^
read_head
The receive buffer is 4096 bytes, and we are sending a character
sequence that repeats every 126 - 47 = 79 bytes. Therefore the offset between
the old data and the new data is 4096 mod 79 = 67.
Instead of the new value 52, we still read the old value 52 - 67 (with
wrapping around from 48 to 127) = 64 = 52 + 12.
I have now applied my proposed fix below, I will run a test for the new
few days and report if this finally solved my problem.
However, since I am not familiar with memory barriers, I would like to
ask you for comments if this is the correct way to solve this problem.
Thank you!
Regards, Christian
drivers/tty/n_tty.c | 20 +++-
1 file changed, 19 insertions(+), 1 deletion(-)
diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
index 89c4cee..831137e 1
Re: [PATCH] n_tty: Add memory barrier to fix race condition in receive path
On Thu, Nov 6, 2014 at 9:56 PM, Greg Kroah-Hartman gre...@linuxfoundation.org wrote: On Thu, Nov 06, 2014 at 08:49:01PM +, Måns Rullgård wrote: Greg Kroah-Hartman gre...@linuxfoundation.org writes: On Thu, Nov 06, 2014 at 12:39:59PM +0100, Christian Riesch wrote: The current implementation of put_tty_queue() causes a race condition when re-arranged by the compiler. On my build with gcc 4.8.3, cross-compiling for ARM, the line *read_buf_addr(ldata, ldata-read_head++) = c; was re-arranged by the compiler to something like x = ldata-read_head ldata-read_head++ *read_buf_addr(ldata, x) = c; which causes a race condition. Invalid data is read if data is read before it is actually written to the read buffer. Really? A compiler can rearange things like that and expect things to actually work? How is that valid? This is actually required by the C spec. There is a sequence point before a function call, after the arguments have been evaluated. Thus all side-effects, such as the post-increment, must be complete before the function is called, just like in the example. There is no re-arranging here. The code is simply wrong. Oh, I didn't know that, thanks a lot for this! Anyway, because of this, no need for the wmb() calls, just rearrange the logic and all should be good, right? I came up with the wmb() stuff after getting scared from reading Documentation/memory-barriers.txt (which I didn't understand) and Documentation/circular-buffers.txt (which I understood partly). But it is actually a circular buffer and circular-buffers.txt says I need memory barriers for circular buffers. Though I do not know how function calls fit into this picture. Christian, can you test that instead? Sure, but it will probably not happen before Monday. Thanks a lot for your help! Christian -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] n_tty: Add memory barrier to fix race condition in receive path
[sent again due to stupid HTML mail problems, sorry] On Thu, Nov 6, 2014 at 11:54 PM, Måns Rullgård m...@mansr.com wrote: Greg Kroah-Hartman gre...@linuxfoundation.org writes: On Thu, Nov 06, 2014 at 10:12:54PM +, Måns Rullgård wrote: Greg Kroah-Hartman gre...@linuxfoundation.org writes: On Thu, Nov 06, 2014 at 09:38:59PM +, Måns Rullgård wrote: Greg Kroah-Hartman gre...@linuxfoundation.org writes: On Thu, Nov 06, 2014 at 09:01:36PM +, Måns Rullgård wrote: Greg Kroah-Hartman gre...@linuxfoundation.org writes: On Thu, Nov 06, 2014 at 08:49:01PM +, Måns Rullgård wrote: Greg Kroah-Hartman gre...@linuxfoundation.org writes: On Thu, Nov 06, 2014 at 12:39:59PM +0100, Christian Riesch wrote: The current implementation of put_tty_queue() causes a race condition when re-arranged by the compiler. On my build with gcc 4.8.3, cross-compiling for ARM, the line *read_buf_addr(ldata, ldata-read_head++) = c; was re-arranged by the compiler to something like x = ldata-read_head ldata-read_head++ *read_buf_addr(ldata, x) = c; which causes a race condition. Invalid data is read if data is read before it is actually written to the read buffer. Really? A compiler can rearange things like that and expect things to actually work? How is that valid? This is actually required by the C spec. There is a sequence point before a function call, after the arguments have been evaluated. Thus all side-effects, such as the post-increment, must be complete before the function is called, just like in the example. There is no re-arranging here. The code is simply wrong. Ah, ok, time to dig out the C spec... Anyway, because of this, no need for the wmb() calls, just rearrange the logic and all should be good, right? Christian, can you test that instead? Weakly ordered SMP systems probably need some kind of barrier. I didn't look at it carefully. It shouldn't need a barier, as it is a sequence point with the function call. Well, it's an inline function, but that shouldn't matter here, right? Sequence points say nothing about the order in which stores become visible to other CPUs. That's why there are barrier instructions. Yes, but order matters. If I write code that does: 100x = ldata-read_head; 101ldata-read_head[x SOME_VALUE] = y; 102ldata-read_head++; the compiler can not reorder lines 102 and 101 just because it feels like it, right? Or is it time to go spend some reading of the C spec again... The compiler can't. The hardware can. All the hardware promises is that at some unspecified time in the future, both memory locations will have the correct values. Another CPU might see 'read_head' updated before it sees the corresponding data value. A wmb() between the writes forces the CPU to complete preceding stores before it begins subsequent ones. Yes, sorry, I'm not talking about other CPUs and what they see, I'm talking about the local one. I'm not assuming that this is SMP safe at all. If it is supposed to be, then yes, we do have problems, but there should be a lock _somewhere_ protecting this. Within the confines of a single CPU + memory, barriers are never needed. The moment another CPU or master-capable peripheral enters the mix, proper ordering must be enforced somehow. If the buffer is already protected by a lock of some kind, this will provide the necessary barriers, so nothing further is necessary. If it's a lock-less design, there will need to be barriers somewhere. It was changed to lock-less with 3.12 in commit 6d76bd2618535c581f1673047b8341fd291abc67 (n_tty: Make N_TTY ldisc receive path lockless). So I will try to read the memory barrier docs again. Of course my little ARM system is no SMP system, but I guess this should also be fixed for the SMP case, right? Thanks, Christian -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] n_tty: Add memory barrier to fix race condition in receive path
The current implementation of put_tty_queue() causes a race condition
when re-arranged by the compiler.
On my build with gcc 4.8.3, cross-compiling for ARM, the line
*read_buf_addr(ldata, ldata-read_head++) = c;
was re-arranged by the compiler to something like
x = ldata-read_head
ldata-read_head++
*read_buf_addr(ldata, x) = c;
which causes a race condition. Invalid data is read if data is read
before it is actually written to the read buffer.
This race condition was introduced with commit
6d76bd2618535c581f1673047b8341fd291abc67 (n_tty: Make N_TTY ldisc receive
path lockless).
This patch adds memory barriers to resolve this race condition.
Signed-off-by: Christian Riesch christian.rie...@omicron.at
Cc: Peter Hurley pe...@hurleysoftware.com
Cc: sta...@vger.kernel.org
---
Hi all,
I noticed that since an upgrade to kernel 3.12 my ARM device communicating
via serial port with a GPS receiver module reports frequent communication
errors.
After several attempts to resolve these problems I created a small
test setup. This setup utilizes a small microcontroller that just sends
data via serial port to the ARM processor using 9600o1.
The code on the microcontroller looks like this:
char c = 48;
while (1) {
if c 126 then c = 48
send character c
c++
}
On the ARM/Linux side I ran the serial port in non-canonical mode,
received the data and checked if the data is what we expect it to be:
struct termios tio;
memset(tio, 0, sizeof(tio));
tio.c_cflag = CREAD | CLOCAL | B9600 | CS8 | PARENB | PARODD;
tio.c_iflag = INPCK;
tio.c_cc[VTIME] = 0;
tio.c_cc[VMIN] = 0;
tcsetattr(fd, TCSANOW, tio);
...
c = 48;
while (1) {
...
poll(pfds, 1, 1000);
if (pfds[0].revents POLLIN) {
ret = read(fd, buf, 200);
for (i = 0; i ret; i++) {
c++;
if (c 126)
c = 48;
if (c != buf[i]) {
printf(expected %d - received %d, ret = %d, i = %d\n,
c, buf[i], ret, i);
c = buf[i];
}
}
}
}
I ran this test for about 5 days, the result was:
expected 51 - received 63, ret = 11, i = 10
expected 64 - received 52, ret = 13, i = 0
expected 64 - received 76, ret = 11, i = 10
expected 77 - received 65, ret = 5, i = 0
expected 105 - received 117, ret = 18, i = 17
expected 118 - received 106, ret = 6, i = 0
expected 120 - received 53, ret = 16, i = 15
expected 54 - received 121, ret = 8, i = 0
expected 105 - received 117, ret = 3, i = 2
expected 118 - received 106, ret = 5, i = 0
expected 79 - received 91, ret = 20, i = 19
expected 92 - received 80, ret = 4, i = 0
expected 72 - received 84, ret = 15, i = 14
expected 85 - received 73, ret = 9, i = 0
expected 54 - received 66, ret = 13, i = 12
expected 67 - received 55, ret = 3, i = 0
expected 86 - received 98, ret = 25, i = 24
expected 99 - received 87, ret = 15, i = 0
expected 86 - received 98, ret = 14, i = 13
expected 99 - received 87, ret = 42, i = 0
expected 93 - received 105, ret = 34, i = 33
expected 106 - received 94, ret = 6, i = 0
expected 92 - received 104, ret = 16, i = 15
expected 105 - received 93, ret = 8, i = 0
expected 53 - received 65, ret = 8, i = 7
expected 66 - received 54, ret = 8, i = 0
The first line shows that we expected buf[i] to be 51, actually we
received 63. We therefore set c = 63, consequently we expect 64 as the
next character. But we receive 52, so everything is back to normal. So
no bytes are missing, no additional bytes are received, there is just
a single byte with a wrong content.
We see that always the last byte in buf is affected, i.e. buf[ret - 1].
Furthermore we see that the wrong byte is always off by 12, i.e. instead
of 51 we received 63 (63 - 51 = 12), instead of 64 we received 76
(76 - 64 = 12) etc.
The race that I described above in the commit message exactly results in
such a behavior.
In the example below read_head was already incremented but the new content
has not yet been written to ldata-read_buf.
48 49 50 51 64 65 66 67
^^
read_head
The receive buffer is 4096 bytes, and we are sending a character
sequence that repeats every 126 - 47 = 79 bytes. Therefore the offset between
the old data and the new data is 4096 mod 79 = 67.
Instead of the new value 52, we still read the old value 52 - 67 (with
wrapping around from 48 to 127) = 64 = 52 + 12.
I have now applied my proposed fix below, I will run a test for the new
few days and report if this finally solved my problem.
However, since I am not familiar with memory barriers, I would like to
ask you for comments if this is the correct way to solve this problem.
Thank you!
Regards, Christian
drivers/tty/n_tty.c | 20 +++-
1 file changed, 19 insertions(+), 1 deletion(-)
diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
Re: [PATCH 1/1] mtd: cfi_cmdset_0002:add fixup for Micron M29EW after erase operation
On Mon, Sep 1, 2014 at 4:20 AM, bpqw wrote:
> For Micron M29EW,20ms delay is needed after erase operation.
>
> Signed-off-by: BeanHuo
> ---
> drivers/mtd/chips/cfi_cmdset_0002.c | 11 +++
> 1 file changed, 11 insertions(+)
>
> diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c
> b/drivers/mtd/chips/cfi_cmdset_0002.c
> index 5a4bfe3..9b0de91 100644
> --- a/drivers/mtd/chips/cfi_cmdset_0002.c
> +++ b/drivers/mtd/chips/cfi_cmdset_0002.c
> @@ -509,6 +509,16 @@ static void cfi_fixup_m29ew_delay_after_resume(struct
> cfi_private *cfi)
> cfi_udelay(500);
> }
>
> +static void cfi_fixup_m29ew_delay_after_erase(struct cfi_private *cfi)
> +{
> + /*
> +* Resolving the Delay After ERASE Issue @low temperature.
> +* 20ms delay is needed after erase operation.
> +*/
> + if (is_m29ew(cfi))
> + cfi_udelay(2);
> +}
> +
> struct mtd_info *cfi_cmdset_0002(struct map_info *map, int primary) {
> struct cfi_private *cfi = map->fldrv_priv; @@ -2397,6 +2407,7 @@
> static int __xipram do_erase_oneblock(struct map_info *map, struct flchip
> *chip,
Missing line break? The patch does not apply.
Christian
> ret = -EIO;
> }
>
> + cfi_fixup_m29ew_delay_after_erase(cfi);
> chip->state = FL_READY;
> DISABLE_VPP(map);
> put_chip(map, chip, adr);
> --
> 1.7.9.5
>
> __
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/1] mtd: cfi_cmdset_0002:add fixup for Micron M29EW after erase operation
On Mon, Sep 1, 2014 at 4:20 AM, bpqw wrote:
> For Micron M29EW,20ms delay is needed after erase operation.
>
> Signed-off-by: BeanHuo
> ---
> drivers/mtd/chips/cfi_cmdset_0002.c | 11 +++
> 1 file changed, 11 insertions(+)
>
> diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c
> b/drivers/mtd/chips/cfi_cmdset_0002.c
> index 5a4bfe3..9b0de91 100644
> --- a/drivers/mtd/chips/cfi_cmdset_0002.c
> +++ b/drivers/mtd/chips/cfi_cmdset_0002.c
> @@ -509,6 +509,16 @@ static void cfi_fixup_m29ew_delay_after_resume(struct
> cfi_private *cfi)
> cfi_udelay(500);
> }
>
> +static void cfi_fixup_m29ew_delay_after_erase(struct cfi_private *cfi)
> +{
> + /*
> +* Resolving the Delay After ERASE Issue @low temperature.
> +* 20ms delay is needed after erase operation.
> +*/
> + if (is_m29ew(cfi))
> + cfi_udelay(2);
> +}
> +
> struct mtd_info *cfi_cmdset_0002(struct map_info *map, int primary) {
> struct cfi_private *cfi = map->fldrv_priv; @@ -2397,6 +2407,7 @@
> static int __xipram do_erase_oneblock(struct map_info *map, struct flchip
> *chip,
> ret = -EIO;
> }
>
> + cfi_fixup_m29ew_delay_after_erase(cfi);
> chip->state = FL_READY;
> DISABLE_VPP(map);
> put_chip(map, chip, adr);
> --
> 1.7.9.5
>
> __
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/1] mtd: cfi_cmdset_0002:add fixup for Micron M29EW after erase operation
Hi,
On Mon, Sep 1, 2014 at 4:20 AM, bpqw wrote:
> For Micron M29EW,20ms delay is needed after erase operation.
Is there a datasheet/application note/technical note from Micron
describing this issue? Like the TN-13-07 for the other M29EW fixes?
Are all M29EW types affected?
Thanks,
Christian
>
> Signed-off-by: BeanHuo
> ---
> drivers/mtd/chips/cfi_cmdset_0002.c | 11 +++
> 1 file changed, 11 insertions(+)
>
> diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c
> b/drivers/mtd/chips/cfi_cmdset_0002.c
> index 5a4bfe3..9b0de91 100644
> --- a/drivers/mtd/chips/cfi_cmdset_0002.c
> +++ b/drivers/mtd/chips/cfi_cmdset_0002.c
> @@ -509,6 +509,16 @@ static void cfi_fixup_m29ew_delay_after_resume(struct
> cfi_private *cfi)
> cfi_udelay(500);
> }
>
> +static void cfi_fixup_m29ew_delay_after_erase(struct cfi_private *cfi)
> +{
> + /*
> +* Resolving the Delay After ERASE Issue @low temperature.
> +* 20ms delay is needed after erase operation.
> +*/
> + if (is_m29ew(cfi))
> + cfi_udelay(2);
> +}
> +
> struct mtd_info *cfi_cmdset_0002(struct map_info *map, int primary) {
> struct cfi_private *cfi = map->fldrv_priv; @@ -2397,6 +2407,7 @@
> static int __xipram do_erase_oneblock(struct map_info *map, struct flchip
> *chip,
> ret = -EIO;
> }
>
> + cfi_fixup_m29ew_delay_after_erase(cfi);
> chip->state = FL_READY;
> DISABLE_VPP(map);
> put_chip(map, chip, adr);
> --
> 1.7.9.5
>
> __
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/1] mtd: cfi_cmdset_0002:add fixup for Micron M29EW after erase operation
Hi,
On Mon, Sep 1, 2014 at 4:20 AM, bpqw b...@micron.com wrote:
For Micron M29EW,20ms delay is needed after erase operation.
Is there a datasheet/application note/technical note from Micron
describing this issue? Like the TN-13-07 for the other M29EW fixes?
Are all M29EW types affected?
Thanks,
Christian
Signed-off-by: BeanHuo b...@micron.com
---
drivers/mtd/chips/cfi_cmdset_0002.c | 11 +++
1 file changed, 11 insertions(+)
diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c
b/drivers/mtd/chips/cfi_cmdset_0002.c
index 5a4bfe3..9b0de91 100644
--- a/drivers/mtd/chips/cfi_cmdset_0002.c
+++ b/drivers/mtd/chips/cfi_cmdset_0002.c
@@ -509,6 +509,16 @@ static void cfi_fixup_m29ew_delay_after_resume(struct
cfi_private *cfi)
cfi_udelay(500);
}
+static void cfi_fixup_m29ew_delay_after_erase(struct cfi_private *cfi)
+{
+ /*
+* Resolving the Delay After ERASE Issue @low temperature.
+* 20ms delay is needed after erase operation.
+*/
+ if (is_m29ew(cfi))
+ cfi_udelay(2);
+}
+
struct mtd_info *cfi_cmdset_0002(struct map_info *map, int primary) {
struct cfi_private *cfi = map-fldrv_priv; @@ -2397,6 +2407,7 @@
static int __xipram do_erase_oneblock(struct map_info *map, struct flchip
*chip,
ret = -EIO;
}
+ cfi_fixup_m29ew_delay_after_erase(cfi);
chip-state = FL_READY;
DISABLE_VPP(map);
put_chip(map, chip, adr);
--
1.7.9.5
__
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/1] mtd: cfi_cmdset_0002:add fixup for Micron M29EW after erase operation
On Mon, Sep 1, 2014 at 4:20 AM, bpqw b...@micron.com wrote:
For Micron M29EW,20ms delay is needed after erase operation.
Signed-off-by: BeanHuo b...@micron.com
---
drivers/mtd/chips/cfi_cmdset_0002.c | 11 +++
1 file changed, 11 insertions(+)
diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c
b/drivers/mtd/chips/cfi_cmdset_0002.c
index 5a4bfe3..9b0de91 100644
--- a/drivers/mtd/chips/cfi_cmdset_0002.c
+++ b/drivers/mtd/chips/cfi_cmdset_0002.c
@@ -509,6 +509,16 @@ static void cfi_fixup_m29ew_delay_after_resume(struct
cfi_private *cfi)
cfi_udelay(500);
}
+static void cfi_fixup_m29ew_delay_after_erase(struct cfi_private *cfi)
+{
+ /*
+* Resolving the Delay After ERASE Issue @low temperature.
+* 20ms delay is needed after erase operation.
+*/
+ if (is_m29ew(cfi))
+ cfi_udelay(2);
+}
+
struct mtd_info *cfi_cmdset_0002(struct map_info *map, int primary) {
struct cfi_private *cfi = map-fldrv_priv; @@ -2397,6 +2407,7 @@
static int __xipram do_erase_oneblock(struct map_info *map, struct flchip
*chip,
ret = -EIO;
}
+ cfi_fixup_m29ew_delay_after_erase(cfi);
chip-state = FL_READY;
DISABLE_VPP(map);
put_chip(map, chip, adr);
--
1.7.9.5
__
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/1] mtd: cfi_cmdset_0002:add fixup for Micron M29EW after erase operation
On Mon, Sep 1, 2014 at 4:20 AM, bpqw b...@micron.com wrote:
For Micron M29EW,20ms delay is needed after erase operation.
Signed-off-by: BeanHuo b...@micron.com
---
drivers/mtd/chips/cfi_cmdset_0002.c | 11 +++
1 file changed, 11 insertions(+)
diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c
b/drivers/mtd/chips/cfi_cmdset_0002.c
index 5a4bfe3..9b0de91 100644
--- a/drivers/mtd/chips/cfi_cmdset_0002.c
+++ b/drivers/mtd/chips/cfi_cmdset_0002.c
@@ -509,6 +509,16 @@ static void cfi_fixup_m29ew_delay_after_resume(struct
cfi_private *cfi)
cfi_udelay(500);
}
+static void cfi_fixup_m29ew_delay_after_erase(struct cfi_private *cfi)
+{
+ /*
+* Resolving the Delay After ERASE Issue @low temperature.
+* 20ms delay is needed after erase operation.
+*/
+ if (is_m29ew(cfi))
+ cfi_udelay(2);
+}
+
struct mtd_info *cfi_cmdset_0002(struct map_info *map, int primary) {
struct cfi_private *cfi = map-fldrv_priv; @@ -2397,6 +2407,7 @@
static int __xipram do_erase_oneblock(struct map_info *map, struct flchip
*chip,
Missing line break? The patch does not apply.
Christian
ret = -EIO;
}
+ cfi_fixup_m29ew_delay_after_erase(cfi);
chip-state = FL_READY;
DISABLE_VPP(map);
put_chip(map, chip, adr);
--
1.7.9.5
__
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: USB_MUSB_DA8XX with g_multi.ko
Hi Jon, On Thu, Jun 26, 2014 at 4:46 AM, Jon Ringle wrote: > The only thing I needed to do was to remove the > BROKEN dependency on USB_MUSB_DA8XX. Is there anything else I would > have to do besides just removing BROKEN to get this accepted back into > the kernel source tree? I tried to remove the BROKEN dependency in [1], but without success. See Felipe's response to my patch for more info. There was also another discussion about this, see [2]. Christian [1] http://marc.info/?t=1394061=1=2 [2] http://marc.info/?t=13952576757=1=2 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: USB_MUSB_DA8XX with g_multi.ko
Hi Jon, On Thu, Jun 26, 2014 at 4:46 AM, Jon Ringle j...@ringle.org wrote: The only thing I needed to do was to remove the BROKEN dependency on USB_MUSB_DA8XX. Is there anything else I would have to do besides just removing BROKEN to get this accepted back into the kernel source tree? I tried to remove the BROKEN dependency in [1], but without success. See Felipe's response to my patch for more info. There was also another discussion about this, see [2]. Christian [1] http://marc.info/?t=1394061r=1w=2 [2] http://marc.info/?t=13952576757r=1w=2 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH RFC net-next v1 0/9] ptp: dynamic pin control
Hi Richard, --On March 08, 2014 20:42 +0100 Richard Cochran wrote: This patch series introduces a way of changing the auxiliary PTP Hardware Clock functions (periodic output signals and time stamping external signals) at run time. In the past on the netdev list, we have discussed other ways to handle this, such as module parameters and ethtool. This series implements a new PHC ioctl because that is the most natural way. Users already activate the auxiliary functions via the ioctls. The sysfs interface has also been expanded so that the pin configuration can be programmed using shell scripts. I did a few tests on one of my boards, everything works so far! Thanks for the patchset, I like it! The first patch adds the new ioctls. The PHC subsystem does most of the work of maintaining the function-to-pin mapping. Drivers will only need to allocate and initialize a pin configuration table and also provide a new method that validates a particular assignment. Patches 5 and 6 just clean up a couple of issues in the phyter driver, and the remaining patches actually hook the phyter's pins into the new system. Comments and questions are most welcome. Do you think it is possible to extend this in the future, e.g. for selecting the polarity of periodic output signals or for time stamping of external signals (rising edge/falling edge), or duty cycles of the periodic signal other than 50%? How could this be done? Using the reserved fields in struct ptp_pin_desc? Do you think the concept allows an extension for single pulse output, e.g. programming a pin to output a single pulse at a given time, as supported by the DP83640? If several DP83640 are connected together with the calibration function, only the GPIOs of the master device can be used, right? I guess this could also be extended in the future to use the GPIOs of all DP83640, right? Or do you see a problem with your concept here? Best regards, Christian -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH RFC net-next v1 0/9] ptp: dynamic pin control
Hi Richard, --On March 10, 2014 14:42 +0100 Richard Cochran wrote: On Mon, Mar 10, 2014 at 12:52:57PM +, Sørensen, Stefan wrote: What are the n_ext_ts and n_per_out supposed to be set to now? The number of pins configured for the relevant function or the number of channels that are available for the function? They are the number of available functions. This is not necessarily the same as the number of pins. Many chips have more functions than pins. The implementation is limited to a single function for each pin - the dp83640 supports an ext_ts and several periodic outputs on the same pin, but I do not see that many real-world uses. Yes, it does support different functions on a single pin, but not at the same time. You have to choose what function you want on each pin, and that is what the new ioctl lets you do. I did not change the n_per_out in this series, Why not? I think it would be a good idea to set both n_ext_ts and n_per_out to the correct value right from the start (8, right?). Christian but once this basic support gets merged, then you will be able to increase n_per_out for the dp83640 with little extra effort. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH RFC net-next v1 0/9] ptp: dynamic pin control
Hi Richard, --On March 10, 2014 14:42 +0100 Richard Cochran richardcoch...@gmail.com wrote: On Mon, Mar 10, 2014 at 12:52:57PM +, Sørensen, Stefan wrote: What are the n_ext_ts and n_per_out supposed to be set to now? The number of pins configured for the relevant function or the number of channels that are available for the function? They are the number of available functions. This is not necessarily the same as the number of pins. Many chips have more functions than pins. The implementation is limited to a single function for each pin - the dp83640 supports an ext_ts and several periodic outputs on the same pin, but I do not see that many real-world uses. Yes, it does support different functions on a single pin, but not at the same time. You have to choose what function you want on each pin, and that is what the new ioctl lets you do. I did not change the n_per_out in this series, Why not? I think it would be a good idea to set both n_ext_ts and n_per_out to the correct value right from the start (8, right?). Christian but once this basic support gets merged, then you will be able to increase n_per_out for the dp83640 with little extra effort. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH RFC net-next v1 0/9] ptp: dynamic pin control
Hi Richard, --On March 08, 2014 20:42 +0100 Richard Cochran richardcoch...@gmail.com wrote: This patch series introduces a way of changing the auxiliary PTP Hardware Clock functions (periodic output signals and time stamping external signals) at run time. In the past on the netdev list, we have discussed other ways to handle this, such as module parameters and ethtool. This series implements a new PHC ioctl because that is the most natural way. Users already activate the auxiliary functions via the ioctls. The sysfs interface has also been expanded so that the pin configuration can be programmed using shell scripts. I did a few tests on one of my boards, everything works so far! Thanks for the patchset, I like it! The first patch adds the new ioctls. The PHC subsystem does most of the work of maintaining the function-to-pin mapping. Drivers will only need to allocate and initialize a pin configuration table and also provide a new method that validates a particular assignment. Patches 5 and 6 just clean up a couple of issues in the phyter driver, and the remaining patches actually hook the phyter's pins into the new system. Comments and questions are most welcome. Do you think it is possible to extend this in the future, e.g. for selecting the polarity of periodic output signals or for time stamping of external signals (rising edge/falling edge), or duty cycles of the periodic signal other than 50%? How could this be done? Using the reserved fields in struct ptp_pin_desc? Do you think the concept allows an extension for single pulse output, e.g. programming a pin to output a single pulse at a given time, as supported by the DP83640? If several DP83640 are connected together with the calibration function, only the GPIOs of the master device can be used, right? I guess this could also be extended in the future to use the GPIOs of all DP83640, right? Or do you see a problem with your concept here? Best regards, Christian -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH RFC net-next v1 1/9] ptp: introduce programmable pins.
Hi Richard,
--On March 08, 2014 20:42 +0100 Richard Cochran
wrote:
This patch adds a pair of new ioctls to the PTP Hardware Clock device
interface. Using the ioctls, user space programs can query each pin to
find out its current function and also reprogram a different function
if desired.
Signed-off-by: Richard Cochran
---
drivers/ptp/ptp_chardev.c| 133
+- drivers/ptp/ptp_clock.c
| 23 +++
drivers/ptp/ptp_private.h|4 ++
include/linux/ptp_clock_kernel.h | 33 ++
include/uapi/linux/ptp_clock.h | 39 ++-
5 files changed, 230 insertions(+), 2 deletions(-)
diff --git a/drivers/ptp/ptp_chardev.c b/drivers/ptp/ptp_chardev.c
index 34a0c60..dc13d1e 100644
--- a/drivers/ptp/ptp_chardev.c
+++ b/drivers/ptp/ptp_chardev.c
[...]
+int ptp_setpin(struct ptp_clock *ptp, unsigned int pin,
+ enum ptp_pin_function func, unsigned int chan)
+{
+ struct ptp_clock_info *info = ptp->info;
+ struct ptp_pin_desc *pin1 = NULL, *pin2 = >pin_config[pin];
+ unsigned int i;
+
+ /* Check to see if any other pin previously had this function. */
+ if (mutex_lock_interruptible(>pincfg_mux))
+ return -ERESTARTSYS;
+ for (i = 0; i < info->n_pins; i++) {
+ if (info->pin_config[i].func == func &&
+ info->pin_config[i].chan == chan) {
+ pin1 = >pin_config[i];
+ break;
+ }
+ }
+ mutex_unlock(>pincfg_mux);
+ if (pin1 && i == pin)
+ return 0;
+
+ /* Check the desired function and channel. */
+ switch (func) {
+ case PTP_PF_NONE:
+ break;
+ case PTP_PF_EXTTS:
+ if (chan >= info->n_ext_ts)
+ return -EINVAL;
+ break;
+ case PTP_PF_PEROUT:
+ if (chan >= info->n_per_out)
+ return -EINVAL;
+ break;
+ case PTP_PF_PHYSYNC:
+ pr_err("sorry, cannot reassign the calibration pin\n");
+ return -EINVAL;
+ default:
+ return -EINVAL;
+ }
+
+ if (pin1 && pin1->func == PTP_PF_PHYSYNC) {
+ pr_err("sorry, cannot reprogram the calibration pin\n");
+ return -EINVAL;
Will this ever happen? pin1 && pin1->func == PTP_PF_PHYSYNC means that func
== PTP_PF_PHYSYNC, but in this case you already return -EINVAL a few lines
above.
Christian
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH RFC net-next v1 1/9] ptp: introduce programmable pins.
Hi Richard,
--On March 08, 2014 20:42 +0100 Richard Cochran richardcoch...@gmail.com
wrote:
This patch adds a pair of new ioctls to the PTP Hardware Clock device
interface. Using the ioctls, user space programs can query each pin to
find out its current function and also reprogram a different function
if desired.
Signed-off-by: Richard Cochran richardcoch...@gmail.com
---
drivers/ptp/ptp_chardev.c| 133
+- drivers/ptp/ptp_clock.c
| 23 +++
drivers/ptp/ptp_private.h|4 ++
include/linux/ptp_clock_kernel.h | 33 ++
include/uapi/linux/ptp_clock.h | 39 ++-
5 files changed, 230 insertions(+), 2 deletions(-)
diff --git a/drivers/ptp/ptp_chardev.c b/drivers/ptp/ptp_chardev.c
index 34a0c60..dc13d1e 100644
--- a/drivers/ptp/ptp_chardev.c
+++ b/drivers/ptp/ptp_chardev.c
[...]
+int ptp_setpin(struct ptp_clock *ptp, unsigned int pin,
+ enum ptp_pin_function func, unsigned int chan)
+{
+ struct ptp_clock_info *info = ptp-info;
+ struct ptp_pin_desc *pin1 = NULL, *pin2 = info-pin_config[pin];
+ unsigned int i;
+
+ /* Check to see if any other pin previously had this function. */
+ if (mutex_lock_interruptible(ptp-pincfg_mux))
+ return -ERESTARTSYS;
+ for (i = 0; i info-n_pins; i++) {
+ if (info-pin_config[i].func == func
+ info-pin_config[i].chan == chan) {
+ pin1 = info-pin_config[i];
+ break;
+ }
+ }
+ mutex_unlock(ptp-pincfg_mux);
+ if (pin1 i == pin)
+ return 0;
+
+ /* Check the desired function and channel. */
+ switch (func) {
+ case PTP_PF_NONE:
+ break;
+ case PTP_PF_EXTTS:
+ if (chan = info-n_ext_ts)
+ return -EINVAL;
+ break;
+ case PTP_PF_PEROUT:
+ if (chan = info-n_per_out)
+ return -EINVAL;
+ break;
+ case PTP_PF_PHYSYNC:
+ pr_err(sorry, cannot reassign the calibration pin\n);
+ return -EINVAL;
+ default:
+ return -EINVAL;
+ }
+
+ if (pin1 pin1-func == PTP_PF_PHYSYNC) {
+ pr_err(sorry, cannot reprogram the calibration pin\n);
+ return -EINVAL;
Will this ever happen? pin1 pin1-func == PTP_PF_PHYSYNC means that func
== PTP_PF_PHYSYNC, but in this case you already return -EINVAL a few lines
above.
Christian
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 RESEND 2/2] mtd: Fix the behavior of otp write if there is not enough room for data
Hi Brian,
--On March 06, 2014 00:49 -0800 Brian Norris
wrote:
Hi,
On Wed, Mar 05, 2014 at 09:50:35AM +0100, Christian Riesch wrote:
On March 04, 2014 23:20 -0800 Brian Norris
wrote:
> On Tue, Jan 28, 2014 at 09:29:45AM +0100, Christian Riesch wrote:
>> An OTP write shall write as much data as possible to the OTP memory
>> and return the number of bytes that have actually been written.
>> If no data could be written at all due to lack of OTP memory,
>> return -ENOSPC.
>>
>> Signed-off-by: Christian Riesch
>> Cc: Artem Bityutskiy
>> Cc: Kyungmin Park
>> Cc: Amul Kumar Saha
>> ---
>> drivers/mtd/chips/cfi_cmdset_0001.c | 13 +++--
>> drivers/mtd/devices/mtd_dataflash.c | 13 +
>> drivers/mtd/mtdchar.c |7 +++
>> drivers/mtd/onenand/onenand_base.c | 10 +-
>> 4 files changed, 32 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/mtd/chips/cfi_cmdset_0001.c
>> b/drivers/mtd/chips/cfi_cmdset_0001.c index 7aa581f..cf423a6 100644
>> --- a/drivers/mtd/chips/cfi_cmdset_0001.c
>> +++ b/drivers/mtd/chips/cfi_cmdset_0001.c
>> @@ -2387,8 +2387,17 @@ static int
>> cfi_intelext_write_user_prot_reg(struct mtd_info *mtd, loff_t from,
>>size_t len, size_t *retlen,
>> u_char *buf)
>> {
>> - return cfi_intelext_otp_walk(mtd, from, len, retlen,
>> - buf, do_otp_write, 1);
>> + int ret;
>> +
>> + ret = cfi_intelext_otp_walk(mtd, from, len, retlen,
>> + buf, do_otp_write, 1);
>> +
>> + /* if no data could be written due to lack of OTP memory,
>> + return ENOSPC */
>
> /*
> * Can you use this style of mult-line comments please?
> * It's in Documentation/CodingStyle
> */
>
Ok, I will change that.
>> + if (!ret && len && !(*retlen))
>> + return -ENOSPC;
>
> Couldn't (shouldn't) this check be pushed to the common
> mtd_write_user_prot_reg() helper in mtdcore.c?
Yes, I don't see why this wouldn't work. But I thought the code
would be easier to understand if we return the correct error code as
soon as the error is detected, not using some additional logic in
some other function. What do you think?
No, this is the purpose of the mtd_xxx() wrappers for the mtd->_xxx
implementations. That way we don't have to inconsistently implement the
same checks in every driver. This caught a few bugs for
mtd_{read,write}() when we unified the bounds checking, I think.
Ok, I will change that.
> And once you do that, you
> will see that cfi_intelext_write_user_prot_reg() (and other
> mtd->_write_user_prot_reg() implementations) will never be called with
> len == 0. So this just becomes (in mtdcore.c):
>
> diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
> index 0a7d77e65335..ee6730748f7e 100644
> --- a/drivers/mtd/mtdcore.c
> +++ b/drivers/mtd/mtdcore.c
> @@ -909,11 +909,16 @@ EXPORT_SYMBOL_GPL(mtd_read_fact_prot_reg);
> int mtd_get_user_prot_info(struct mtd_info *mtd, size_t len, size_t
> *retlen, struct otp_info *buf)
> {
> + int ret;
> +
>if (!mtd->_get_user_prot_info)
>return -EOPNOTSUPP;
>if (!len)
>return 0;
> - return mtd->_get_user_prot_info(mtd, len, retlen, buf);
> + ret = mtd->_get_user_prot_info(mtd, len, retlen, buf);
> + if (ret)
> + return ret;
> + return !(*retlen) ? -ENOSPC: 0;
> }
> EXPORT_SYMBOL_GPL(mtd_get_user_prot_info);
>
>
>> +
>> + return ret;
>> }
>>
>> static int cfi_intelext_lock_user_prot_reg(struct mtd_info *mtd,
>> diff --git a/drivers/mtd/devices/mtd_dataflash.c
>> b/drivers/mtd/devices/mtd_dataflash.c index 09c69ce..5236d85 100644
>> --- a/drivers/mtd/devices/mtd_dataflash.c
>> +++ b/drivers/mtd/devices/mtd_dataflash.c
>> @@ -545,14 +545,11 @@ static int dataflash_write_user_otp(struct
>> mtd_info *mtd, struct dataflash*priv = mtd->priv;
>>int status;
>>
>
> I'm not sure I quite follow the logic for the following hunk. I think
> it deserves some more explanation, either in your commit or in a
> comment. As it stands, you're deleting a comment and potentially
> changing the return code behavior subtly.
>
>> - if (len > 64)
>> - return -EINVAL;
>> -
>> - /* Strictly speaking, we *could* truncate the write ... but
>> - * let's not do that for the only write that's ever possible.
>> - */
>> -
Re: [PATCH v2 RESEND 2/2] mtd: Fix the behavior of otp write if there is not enough room for data
Hi Brian,
--On March 06, 2014 00:49 -0800 Brian Norris computersforpe...@gmail.com
wrote:
Hi,
On Wed, Mar 05, 2014 at 09:50:35AM +0100, Christian Riesch wrote:
On March 04, 2014 23:20 -0800 Brian Norris computersforpe...@gmail.com
wrote:
On Tue, Jan 28, 2014 at 09:29:45AM +0100, Christian Riesch wrote:
An OTP write shall write as much data as possible to the OTP memory
and return the number of bytes that have actually been written.
If no data could be written at all due to lack of OTP memory,
return -ENOSPC.
Signed-off-by: Christian Riesch christian.rie...@omicron.at
Cc: Artem Bityutskiy artem.bityuts...@linux.intel.com
Cc: Kyungmin Park kyungmin.p...@samsung.com
Cc: Amul Kumar Saha amul.s...@samsung.com
---
drivers/mtd/chips/cfi_cmdset_0001.c | 13 +++--
drivers/mtd/devices/mtd_dataflash.c | 13 +
drivers/mtd/mtdchar.c |7 +++
drivers/mtd/onenand/onenand_base.c | 10 +-
4 files changed, 32 insertions(+), 11 deletions(-)
diff --git a/drivers/mtd/chips/cfi_cmdset_0001.c
b/drivers/mtd/chips/cfi_cmdset_0001.c index 7aa581f..cf423a6 100644
--- a/drivers/mtd/chips/cfi_cmdset_0001.c
+++ b/drivers/mtd/chips/cfi_cmdset_0001.c
@@ -2387,8 +2387,17 @@ static int
cfi_intelext_write_user_prot_reg(struct mtd_info *mtd, loff_t from,
size_t len, size_t *retlen,
u_char *buf)
{
- return cfi_intelext_otp_walk(mtd, from, len, retlen,
- buf, do_otp_write, 1);
+ int ret;
+
+ ret = cfi_intelext_otp_walk(mtd, from, len, retlen,
+ buf, do_otp_write, 1);
+
+ /* if no data could be written due to lack of OTP memory,
+ return ENOSPC */
/*
* Can you use this style of mult-line comments please?
* It's in Documentation/CodingStyle
*/
Ok, I will change that.
+ if (!ret len !(*retlen))
+ return -ENOSPC;
Couldn't (shouldn't) this check be pushed to the common
mtd_write_user_prot_reg() helper in mtdcore.c?
Yes, I don't see why this wouldn't work. But I thought the code
would be easier to understand if we return the correct error code as
soon as the error is detected, not using some additional logic in
some other function. What do you think?
No, this is the purpose of the mtd_xxx() wrappers for the mtd-_xxx
implementations. That way we don't have to inconsistently implement the
same checks in every driver. This caught a few bugs for
mtd_{read,write}() when we unified the bounds checking, I think.
Ok, I will change that.
And once you do that, you
will see that cfi_intelext_write_user_prot_reg() (and other
mtd-_write_user_prot_reg() implementations) will never be called with
len == 0. So this just becomes (in mtdcore.c):
diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
index 0a7d77e65335..ee6730748f7e 100644
--- a/drivers/mtd/mtdcore.c
+++ b/drivers/mtd/mtdcore.c
@@ -909,11 +909,16 @@ EXPORT_SYMBOL_GPL(mtd_read_fact_prot_reg);
int mtd_get_user_prot_info(struct mtd_info *mtd, size_t len, size_t
*retlen, struct otp_info *buf)
{
+ int ret;
+
if (!mtd-_get_user_prot_info)
return -EOPNOTSUPP;
if (!len)
return 0;
- return mtd-_get_user_prot_info(mtd, len, retlen, buf);
+ ret = mtd-_get_user_prot_info(mtd, len, retlen, buf);
+ if (ret)
+ return ret;
+ return !(*retlen) ? -ENOSPC: 0;
}
EXPORT_SYMBOL_GPL(mtd_get_user_prot_info);
+
+ return ret;
}
static int cfi_intelext_lock_user_prot_reg(struct mtd_info *mtd,
diff --git a/drivers/mtd/devices/mtd_dataflash.c
b/drivers/mtd/devices/mtd_dataflash.c index 09c69ce..5236d85 100644
--- a/drivers/mtd/devices/mtd_dataflash.c
+++ b/drivers/mtd/devices/mtd_dataflash.c
@@ -545,14 +545,11 @@ static int dataflash_write_user_otp(struct
mtd_info *mtd, struct dataflash*priv = mtd-priv;
int status;
I'm not sure I quite follow the logic for the following hunk. I think
it deserves some more explanation, either in your commit or in a
comment. As it stands, you're deleting a comment and potentially
changing the return code behavior subtly.
- if (len 64)
- return -EINVAL;
-
- /* Strictly speaking, we *could* truncate the write ... but
- * let's not do that for the only write that's ever possible.
- */
- if ((from + len) 64)
- return -EINVAL;
+ if ((from + len) 64) {
+ len = 64 - from;
Why are you reassigning len? Are you trying to undo the comment above,
so that you *can* truncate the write? (It looks like there are other
implmentations which will truncate the write and return -ENOSPC, FWIW.)
Currently we have two kind of implementations: We have
implementations like this one which will refuse to write any data if
the write requests more data to be written than space
Re: [PATCH v2 RESEND 2/2] mtd: Fix the behavior of otp write if there is not enough room for data
Hi Brian,
Thank you very much for your comments on the patch!
--On March 04, 2014 23:20 -0800 Brian Norris
wrote:
Hi Christian,
A few comments below.
On Tue, Jan 28, 2014 at 09:29:45AM +0100, Christian Riesch wrote:
An OTP write shall write as much data as possible to the OTP memory
and return the number of bytes that have actually been written.
If no data could be written at all due to lack of OTP memory,
return -ENOSPC.
Signed-off-by: Christian Riesch
Cc: Artem Bityutskiy
Cc: Kyungmin Park
Cc: Amul Kumar Saha
---
drivers/mtd/chips/cfi_cmdset_0001.c | 13 +++--
drivers/mtd/devices/mtd_dataflash.c | 13 +
drivers/mtd/mtdchar.c |7 +++
drivers/mtd/onenand/onenand_base.c | 10 +-
4 files changed, 32 insertions(+), 11 deletions(-)
diff --git a/drivers/mtd/chips/cfi_cmdset_0001.c
b/drivers/mtd/chips/cfi_cmdset_0001.c index 7aa581f..cf423a6 100644
--- a/drivers/mtd/chips/cfi_cmdset_0001.c
+++ b/drivers/mtd/chips/cfi_cmdset_0001.c
@@ -2387,8 +2387,17 @@ static int
cfi_intelext_write_user_prot_reg(struct mtd_info *mtd, loff_t from,
size_t len, size_t *retlen,
u_char *buf)
{
- return cfi_intelext_otp_walk(mtd, from, len, retlen,
-buf, do_otp_write, 1);
+ int ret;
+
+ ret = cfi_intelext_otp_walk(mtd, from, len, retlen,
+ buf, do_otp_write, 1);
+
+ /* if no data could be written due to lack of OTP memory,
+ return ENOSPC */
/*
* Can you use this style of mult-line comments please?
* It's in Documentation/CodingStyle
*/
Ok, I will change that.
+ if (!ret && len && !(*retlen))
+ return -ENOSPC;
Couldn't (shouldn't) this check be pushed to the common
mtd_write_user_prot_reg() helper in mtdcore.c?
Yes, I don't see why this wouldn't work. But I thought the code would be
easier to understand if we return the correct error code as soon as the
error is detected, not using some additional logic in some other function.
What do you think?
And once you do that, you
will see that cfi_intelext_write_user_prot_reg() (and other
mtd->_write_user_prot_reg() implementations) will never be called with
len == 0. So this just becomes (in mtdcore.c):
diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
index 0a7d77e65335..ee6730748f7e 100644
--- a/drivers/mtd/mtdcore.c
+++ b/drivers/mtd/mtdcore.c
@@ -909,11 +909,16 @@ EXPORT_SYMBOL_GPL(mtd_read_fact_prot_reg);
int mtd_get_user_prot_info(struct mtd_info *mtd, size_t len, size_t
*retlen, struct otp_info *buf)
{
+ int ret;
+
if (!mtd->_get_user_prot_info)
return -EOPNOTSUPP;
if (!len)
return 0;
- return mtd->_get_user_prot_info(mtd, len, retlen, buf);
+ ret = mtd->_get_user_prot_info(mtd, len, retlen, buf);
+ if (ret)
+ return ret;
+ return !(*retlen) ? -ENOSPC: 0;
}
EXPORT_SYMBOL_GPL(mtd_get_user_prot_info);
+
+ return ret;
}
static int cfi_intelext_lock_user_prot_reg(struct mtd_info *mtd,
diff --git a/drivers/mtd/devices/mtd_dataflash.c
b/drivers/mtd/devices/mtd_dataflash.c index 09c69ce..5236d85 100644
--- a/drivers/mtd/devices/mtd_dataflash.c
+++ b/drivers/mtd/devices/mtd_dataflash.c
@@ -545,14 +545,11 @@ static int dataflash_write_user_otp(struct
mtd_info *mtd, struct dataflash *priv = mtd->priv;
int status;
I'm not sure I quite follow the logic for the following hunk. I think it
deserves some more explanation, either in your commit or in a comment.
As it stands, you're deleting a comment and potentially changing the
return code behavior subtly.
- if (len > 64)
- return -EINVAL;
-
- /* Strictly speaking, we *could* truncate the write ... but
-* let's not do that for the only write that's ever possible.
-*/
- if ((from + len) > 64)
- return -EINVAL;
+ if ((from + len) > 64) {
+ len = 64 - from;
Why are you reassigning len? Are you trying to undo the comment above,
so that you *can* truncate the write? (It looks like there are other
implmentations which will truncate the write and return -ENOSPC, FWIW.)
Currently we have two kind of implementations: We have implementations like
this one which will refuse to write any data if the write requests more
data to be written than space is available. And we have implementations
like cfi_intelext_write_user_prot_reg that will truncate the write and
write as much data that is possible (and return the number of bytes that
actually have been written, -ENOSPC shall only be returned if no data could
be written at all).
For a harmonization one of the implementations and their behavior must be
changed. I chose to change it to "write a
Re: [PATCH v2 RESEND 2/2] mtd: Fix the behavior of otp write if there is not enough room for data
Hi Brian,
Thank you very much for your comments on the patch!
--On March 04, 2014 23:20 -0800 Brian Norris computersforpe...@gmail.com
wrote:
Hi Christian,
A few comments below.
On Tue, Jan 28, 2014 at 09:29:45AM +0100, Christian Riesch wrote:
An OTP write shall write as much data as possible to the OTP memory
and return the number of bytes that have actually been written.
If no data could be written at all due to lack of OTP memory,
return -ENOSPC.
Signed-off-by: Christian Riesch christian.rie...@omicron.at
Cc: Artem Bityutskiy artem.bityuts...@linux.intel.com
Cc: Kyungmin Park kyungmin.p...@samsung.com
Cc: Amul Kumar Saha amul.s...@samsung.com
---
drivers/mtd/chips/cfi_cmdset_0001.c | 13 +++--
drivers/mtd/devices/mtd_dataflash.c | 13 +
drivers/mtd/mtdchar.c |7 +++
drivers/mtd/onenand/onenand_base.c | 10 +-
4 files changed, 32 insertions(+), 11 deletions(-)
diff --git a/drivers/mtd/chips/cfi_cmdset_0001.c
b/drivers/mtd/chips/cfi_cmdset_0001.c index 7aa581f..cf423a6 100644
--- a/drivers/mtd/chips/cfi_cmdset_0001.c
+++ b/drivers/mtd/chips/cfi_cmdset_0001.c
@@ -2387,8 +2387,17 @@ static int
cfi_intelext_write_user_prot_reg(struct mtd_info *mtd, loff_t from,
size_t len, size_t *retlen,
u_char *buf)
{
- return cfi_intelext_otp_walk(mtd, from, len, retlen,
-buf, do_otp_write, 1);
+ int ret;
+
+ ret = cfi_intelext_otp_walk(mtd, from, len, retlen,
+ buf, do_otp_write, 1);
+
+ /* if no data could be written due to lack of OTP memory,
+ return ENOSPC */
/*
* Can you use this style of mult-line comments please?
* It's in Documentation/CodingStyle
*/
Ok, I will change that.
+ if (!ret len !(*retlen))
+ return -ENOSPC;
Couldn't (shouldn't) this check be pushed to the common
mtd_write_user_prot_reg() helper in mtdcore.c?
Yes, I don't see why this wouldn't work. But I thought the code would be
easier to understand if we return the correct error code as soon as the
error is detected, not using some additional logic in some other function.
What do you think?
And once you do that, you
will see that cfi_intelext_write_user_prot_reg() (and other
mtd-_write_user_prot_reg() implementations) will never be called with
len == 0. So this just becomes (in mtdcore.c):
diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
index 0a7d77e65335..ee6730748f7e 100644
--- a/drivers/mtd/mtdcore.c
+++ b/drivers/mtd/mtdcore.c
@@ -909,11 +909,16 @@ EXPORT_SYMBOL_GPL(mtd_read_fact_prot_reg);
int mtd_get_user_prot_info(struct mtd_info *mtd, size_t len, size_t
*retlen, struct otp_info *buf)
{
+ int ret;
+
if (!mtd-_get_user_prot_info)
return -EOPNOTSUPP;
if (!len)
return 0;
- return mtd-_get_user_prot_info(mtd, len, retlen, buf);
+ ret = mtd-_get_user_prot_info(mtd, len, retlen, buf);
+ if (ret)
+ return ret;
+ return !(*retlen) ? -ENOSPC: 0;
}
EXPORT_SYMBOL_GPL(mtd_get_user_prot_info);
+
+ return ret;
}
static int cfi_intelext_lock_user_prot_reg(struct mtd_info *mtd,
diff --git a/drivers/mtd/devices/mtd_dataflash.c
b/drivers/mtd/devices/mtd_dataflash.c index 09c69ce..5236d85 100644
--- a/drivers/mtd/devices/mtd_dataflash.c
+++ b/drivers/mtd/devices/mtd_dataflash.c
@@ -545,14 +545,11 @@ static int dataflash_write_user_otp(struct
mtd_info *mtd, struct dataflash *priv = mtd-priv;
int status;
I'm not sure I quite follow the logic for the following hunk. I think it
deserves some more explanation, either in your commit or in a comment.
As it stands, you're deleting a comment and potentially changing the
return code behavior subtly.
- if (len 64)
- return -EINVAL;
-
- /* Strictly speaking, we *could* truncate the write ... but
-* let's not do that for the only write that's ever possible.
-*/
- if ((from + len) 64)
- return -EINVAL;
+ if ((from + len) 64) {
+ len = 64 - from;
Why are you reassigning len? Are you trying to undo the comment above,
so that you *can* truncate the write? (It looks like there are other
implmentations which will truncate the write and return -ENOSPC, FWIW.)
Currently we have two kind of implementations: We have implementations like
this one which will refuse to write any data if the write requests more
data to be written than space is available. And we have implementations
like cfi_intelext_write_user_prot_reg that will truncate the write and
write as much data that is possible (and return the number of bytes that
actually have been written, -ENOSPC shall only be returned if no data could
be written at all).
For a harmonization one of the implementations
Re: Fwd: Ethernet controller not starting
Hi Jon, [Now also cc'ed Prabhakar Lad] --On March 04, 2014 07:34 -0500 Jon Ringle wrote: On Tue, Mar 4, 2014 at 4:06 AM, Christian Riesch wrote: [cc'ed netdev and davinci-linux-open-source] --On March 03, 2014 19:39 -0500 Jon Ringle wrote: On Mon, Mar 3, 2014 at 6:43 PM, Rafael J. Wysocki wrote: On Monday, March 03, 2014 02:41:01 PM Jon Ringle wrote: I'm working on porting an ARM board from linux-3.10 to linux-3.12 (now the latest LTS kernel). I found that Ethernet controller on the board no longer comes up on linux-3.12. I was able to bisect the issue I'm having to the following commit: > 45f0a85c8258741d11bda25c0a5669c06267204a is the first bad commit > commit 45f0a85c8258741d11bda25c0a5669c06267204a > Author: Rafael J. Wysocki > Date: Mon Jun 3 21:49:52 2013 +0200 > [...] Can anyone offer any suggestions on what I should be looking for to fix this on my board? Any pointers to the driver in question? drivers/net/ethernet/ti/davinci_emac.c Hi Jon, I have successfully used the davinci_emac driver on a custom board with an AM1808 SoC with Kernel 3.13 a few weeks ago. So at least 3.13 should work. Did you try more recent kernel versions than 3.12? I have not tried 3.13, but will do so today. Could I get a copy of your .config for comparison purposes? I would like to apologize, apparently my testing with 3.13 was quite bad. Ethernet comes up on my board and works fine after booting, but I can easily reproduce your problem by just doing ifconfig eth0 down /ifconfig eth0 up (which should call emac_dev_stop/emac_dev_open). # ifconfig eth0 up genirq: Flags mismatch irq 33. . So probably one of your start scripts (or dhcp or similar) does some ifconfig down/up, which then fails. I had a look at the code again, emac_dev_open requests the interrupts, whereas emac_dev_stop does not free them. If emac_dev_open is then called again by ifconfig eth0 up, it tries to request the interrupts again, resulting in the error above. I think the problem is a regression caused by Commit 6892b41d9701283085b655c6086fb57a5d63fa47 net: davinci: emac: Convert to devm_* api Signed-off-by: Lad, Prabhakar This patch replaces request_irq() by devm_request_irq() in emac_dev_open and removes free_irq() from emac_dev_stop. But since emac_dev_open is called every time we do an ifconfig eth0 up, it tries to request the interrupts again and again and again (and fails). So I guess the correct solution would be to move the calls of devm_request_irq() to davinci_emac_probe(). I will send a patch to netdev, it solves the problem on my board. Christian -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 RESEND 0/2] mtd: Harmonize implementations of OTP write and _get_{fact, user}_prot_info
Hi,
--On January 28, 2014 09:29 +0100 Christian Riesch
wrote:
Hi all,
In the discussion on my patchset for the OTP support for
drivers/mtd/chips/cfi_cmdset_0002.c [1-5], Artem requested two changes in
the current code of the OTP write functions and the
_get_{fact,user}_prot_info code.
These two patches are an attempt to make the requested changes.
The first patch adds a retlen parameter to the _get_fact_prot_info and
_get_user_prot_info functions and thus harmonizes the implementation
with those of the write and read functions.
The second patch fixes a problem that I earlier addressed in [1]. After
the discussion about this patch on the mtd mailing list, I think that the
correct behavior of the write function should be the one specified in [6]:
Try to write as many bytes as possible and return the number of bytes
that were written. If no data could be written due to lack of OTP memory,
return -ENOSPC.
Artem, would you please have a look at these patches? I would like to know
if I understood you correctly, or if I missed something here. Please note
that I cannot test these patches since I do not have the hardware. The
patches are compile tested only. If these patches are ok, I will also
respin the OTP support patches for cfi_cmdset_0002.
Sorry for bugging you (Artem, mtd Maintainers) again, but I would really
like to get this patchset ([a], [b]) and the OTP support for
cfi_cmdset_0002.c into mainline. Please have a look at the patches and just
tell me if the changes in them are acceptable, or if I should rather post
the OTP support without these two patches. Once I have your answer, I will
respin these patches and my OTP patches, test them again, and submit them.
But without your help I cannot continue.
[a] http://patchwork.ozlabs.org/patch/314624/
[b] http://patchwork.ozlabs.org/patch/314625/
Thank you very much!
Christian
Changes for v2:
- Fixed buggy cfi_intelext_get_fact_prot_info
Thank you!
Best regards,
Christian
[1] http://patchwork.ozlabs.org/patch/239897/
[2] http://patchwork.ozlabs.org/patch/240010/
[3] http://patchwork.ozlabs.org/patch/240007/
[4] http://patchwork.ozlabs.org/patch/240008/
[5] http://patchwork.ozlabs.org/patch/240009/
[6] http://pubs.opengroup.org/onlinepubs/9699919799/functions/write.html
Cc: Artem Bityutskiy
Cc: Kyungmin Park
Cc: Amul Kumar Saha
Cc: Brian Norris
Christian Riesch (2):
mtd: Add a retlen parameter to _get_{fact,user}_prot_info
mtd: Fix the behavior of otp write if there is not enough room for
data
drivers/mtd/chips/cfi_cmdset_0001.c | 44
+++ drivers/mtd/devices/mtd_dataflash.c
| 20 +++-
drivers/mtd/mtdchar.c | 18 ++
drivers/mtd/mtdcore.c | 12 +-
drivers/mtd/mtdpart.c | 14 ++-
drivers/mtd/onenand/onenand_base.c | 40
--- include/linux/mtd/mtd.h |
16 ++---
7 files changed, 89 insertions(+), 75 deletions(-)
--
1.7.9.5
__
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: Ethernet controller not starting
[cc'ed netdev and davinci-linux-open-source] --On March 03, 2014 19:39 -0500 Jon Ringle wrote: On Mon, Mar 3, 2014 at 6:43 PM, Rafael J. Wysocki wrote: On Monday, March 03, 2014 02:41:01 PM Jon Ringle wrote: I'm working on porting an ARM board from linux-3.10 to linux-3.12 (now the latest LTS kernel). I found that Ethernet controller on the board no longer comes up on linux-3.12. I was able to bisect the issue I'm having to the following commit: > 45f0a85c8258741d11bda25c0a5669c06267204a is the first bad commit > commit 45f0a85c8258741d11bda25c0a5669c06267204a > Author: Rafael J. Wysocki > Date: Mon Jun 3 21:49:52 2013 +0200 > > PM / Runtime: Rework the "runtime idle" helper routine > > The "runtime idle" helper routine, rpm_idle(), currently ignores > return values from .runtime_idle() callbacks executed by it. > However, it turns out that many subsystems use > pm_generic_runtime_idle() which checks the return value of the > driver's callback and executes pm_runtime_suspend() for the > device unless that value is not 0. If that logic is moved to > rpm_idle() instead, pm_generic_runtime_idle() can be dropped > and its users will not need any .runtime_idle() callbacks any > more. > > Moreover, the PCI, SCSI, and SATA subsystems' .runtime_idle() > routines, pci_pm_runtime_idle(), scsi_runtime_idle(), and > ata_port_runtime_idle(), respectively, as well as a few drivers' > ones may be simplified if rpm_idle() calls rpm_suspend() after > 0 has been returned by the .runtime_idle() callback executed by > it. > > To reduce overall code bloat, make the changes described above. > > Tested-by: Mika Westerberg > Tested-by: Kevin Hilman > Signed-off-by: Rafael J. Wysocki > Acked-by: Kevin Hilman > Reviewed-by: Ulf Hansson > Acked-by: Alan Stern Can anyone offer any suggestions on what I should be looking for to fix this on my board? Any pointers to the driver in question? drivers/net/ethernet/ti/davinci_emac.c Hi Jon, I have successfully used the davinci_emac driver on a custom board with an AM1808 SoC with Kernel 3.13 a few weeks ago. So at least 3.13 should work. Did you try more recent kernel versions than 3.12? I also get the following output: Starting Network Manager Wait Online... [ 30.946509] davinci_mdio davinci_mdio.0: resetting idled controller [ 30.953220] net net0: attached PHY driver [Generic PHY] (mii_bus:phy_addr=davinci_mdio-0:00, id=7c0f1) [ 30.962938] IPv6: ADDRCONF(NETDEV_UP): net0: link is not ready [ 31.082087] genirq: Flags mismatch irq 33. (net0) vs. (net0) Is it correct that this warning can only appear in case of shared interrupts? See kernel/irq/manage.c. Since we don't use shared interrupts here: Are you sure your board configuration is correct with regard to emac interrupts? Is your configuration using device tree or not (my configuration is a none-devicetree one)? I also wonder why your network device is net0, on my board it's eth0, maybe this triggers some bug? Christian -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Ethernet controller not starting
[cc'ed netdev and davinci-linux-open-source] --On March 03, 2014 19:39 -0500 Jon Ringle j...@ringle.org wrote: On Mon, Mar 3, 2014 at 6:43 PM, Rafael J. Wysocki r...@rjwysocki.net wrote: On Monday, March 03, 2014 02:41:01 PM Jon Ringle wrote: I'm working on porting an ARM board from linux-3.10 to linux-3.12 (now the latest LTS kernel). I found that Ethernet controller on the board no longer comes up on linux-3.12. I was able to bisect the issue I'm having to the following commit: 45f0a85c8258741d11bda25c0a5669c06267204a is the first bad commit commit 45f0a85c8258741d11bda25c0a5669c06267204a Author: Rafael J. Wysocki rafael.j.wyso...@intel.com Date: Mon Jun 3 21:49:52 2013 +0200 PM / Runtime: Rework the runtime idle helper routine The runtime idle helper routine, rpm_idle(), currently ignores return values from .runtime_idle() callbacks executed by it. However, it turns out that many subsystems use pm_generic_runtime_idle() which checks the return value of the driver's callback and executes pm_runtime_suspend() for the device unless that value is not 0. If that logic is moved to rpm_idle() instead, pm_generic_runtime_idle() can be dropped and its users will not need any .runtime_idle() callbacks any more. Moreover, the PCI, SCSI, and SATA subsystems' .runtime_idle() routines, pci_pm_runtime_idle(), scsi_runtime_idle(), and ata_port_runtime_idle(), respectively, as well as a few drivers' ones may be simplified if rpm_idle() calls rpm_suspend() after 0 has been returned by the .runtime_idle() callback executed by it. To reduce overall code bloat, make the changes described above. Tested-by: Mika Westerberg mika.westerb...@linux.intel.com Tested-by: Kevin Hilman khil...@linaro.org Signed-off-by: Rafael J. Wysocki rafael.j.wyso...@intel.com Acked-by: Kevin Hilman khil...@linaro.org Reviewed-by: Ulf Hansson ulf.hans...@linaro.org Acked-by: Alan Stern st...@rowland.harvard.edu Can anyone offer any suggestions on what I should be looking for to fix this on my board? Any pointers to the driver in question? drivers/net/ethernet/ti/davinci_emac.c Hi Jon, I have successfully used the davinci_emac driver on a custom board with an AM1808 SoC with Kernel 3.13 a few weeks ago. So at least 3.13 should work. Did you try more recent kernel versions than 3.12? I also get the following output: Starting Network Manager Wait Online... [ 30.946509] davinci_mdio davinci_mdio.0: resetting idled controller [ 30.953220] net net0: attached PHY driver [Generic PHY] (mii_bus:phy_addr=davinci_mdio-0:00, id=7c0f1) [ 30.962938] IPv6: ADDRCONF(NETDEV_UP): net0: link is not ready [ 31.082087] genirq: Flags mismatch irq 33. (net0) vs. (net0) Is it correct that this warning can only appear in case of shared interrupts? See kernel/irq/manage.c. Since we don't use shared interrupts here: Are you sure your board configuration is correct with regard to emac interrupts? Is your configuration using device tree or not (my configuration is a none-devicetree one)? I also wonder why your network device is net0, on my board it's eth0, maybe this triggers some bug? Christian -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 RESEND 0/2] mtd: Harmonize implementations of OTP write and _get_{fact, user}_prot_info
Hi,
--On January 28, 2014 09:29 +0100 Christian Riesch
christian.rie...@omicron.at wrote:
Hi all,
In the discussion on my patchset for the OTP support for
drivers/mtd/chips/cfi_cmdset_0002.c [1-5], Artem requested two changes in
the current code of the OTP write functions and the
_get_{fact,user}_prot_info code.
These two patches are an attempt to make the requested changes.
The first patch adds a retlen parameter to the _get_fact_prot_info and
_get_user_prot_info functions and thus harmonizes the implementation
with those of the write and read functions.
The second patch fixes a problem that I earlier addressed in [1]. After
the discussion about this patch on the mtd mailing list, I think that the
correct behavior of the write function should be the one specified in [6]:
Try to write as many bytes as possible and return the number of bytes
that were written. If no data could be written due to lack of OTP memory,
return -ENOSPC.
Artem, would you please have a look at these patches? I would like to know
if I understood you correctly, or if I missed something here. Please note
that I cannot test these patches since I do not have the hardware. The
patches are compile tested only. If these patches are ok, I will also
respin the OTP support patches for cfi_cmdset_0002.
Sorry for bugging you (Artem, mtd Maintainers) again, but I would really
like to get this patchset ([a], [b]) and the OTP support for
cfi_cmdset_0002.c into mainline. Please have a look at the patches and just
tell me if the changes in them are acceptable, or if I should rather post
the OTP support without these two patches. Once I have your answer, I will
respin these patches and my OTP patches, test them again, and submit them.
But without your help I cannot continue.
[a] http://patchwork.ozlabs.org/patch/314624/
[b] http://patchwork.ozlabs.org/patch/314625/
Thank you very much!
Christian
Changes for v2:
- Fixed buggy cfi_intelext_get_fact_prot_info
Thank you!
Best regards,
Christian
[1] http://patchwork.ozlabs.org/patch/239897/
[2] http://patchwork.ozlabs.org/patch/240010/
[3] http://patchwork.ozlabs.org/patch/240007/
[4] http://patchwork.ozlabs.org/patch/240008/
[5] http://patchwork.ozlabs.org/patch/240009/
[6] http://pubs.opengroup.org/onlinepubs/9699919799/functions/write.html
Cc: Artem Bityutskiy artem.bityuts...@linux.intel.com
Cc: Kyungmin Park kyungmin.p...@samsung.com
Cc: Amul Kumar Saha amul.s...@samsung.com
Cc: Brian Norris computersforpe...@gmail.com
Christian Riesch (2):
mtd: Add a retlen parameter to _get_{fact,user}_prot_info
mtd: Fix the behavior of otp write if there is not enough room for
data
drivers/mtd/chips/cfi_cmdset_0001.c | 44
+++ drivers/mtd/devices/mtd_dataflash.c
| 20 +++-
drivers/mtd/mtdchar.c | 18 ++
drivers/mtd/mtdcore.c | 12 +-
drivers/mtd/mtdpart.c | 14 ++-
drivers/mtd/onenand/onenand_base.c | 40
--- include/linux/mtd/mtd.h |
16 ++---
7 files changed, 89 insertions(+), 75 deletions(-)
--
1.7.9.5
__
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: Fwd: Ethernet controller not starting
Hi Jon, [Now also cc'ed Prabhakar Lad] --On March 04, 2014 07:34 -0500 Jon Ringle j...@ringle.org wrote: On Tue, Mar 4, 2014 at 4:06 AM, Christian Riesch christian.rie...@omicron.at wrote: [cc'ed netdev and davinci-linux-open-source] --On March 03, 2014 19:39 -0500 Jon Ringle j...@ringle.org wrote: On Mon, Mar 3, 2014 at 6:43 PM, Rafael J. Wysocki r...@rjwysocki.net wrote: On Monday, March 03, 2014 02:41:01 PM Jon Ringle wrote: I'm working on porting an ARM board from linux-3.10 to linux-3.12 (now the latest LTS kernel). I found that Ethernet controller on the board no longer comes up on linux-3.12. I was able to bisect the issue I'm having to the following commit: 45f0a85c8258741d11bda25c0a5669c06267204a is the first bad commit commit 45f0a85c8258741d11bda25c0a5669c06267204a Author: Rafael J. Wysocki rafael.j.wyso...@intel.com Date: Mon Jun 3 21:49:52 2013 +0200 [...] Can anyone offer any suggestions on what I should be looking for to fix this on my board? Any pointers to the driver in question? drivers/net/ethernet/ti/davinci_emac.c Hi Jon, I have successfully used the davinci_emac driver on a custom board with an AM1808 SoC with Kernel 3.13 a few weeks ago. So at least 3.13 should work. Did you try more recent kernel versions than 3.12? I have not tried 3.13, but will do so today. Could I get a copy of your .config for comparison purposes? I would like to apologize, apparently my testing with 3.13 was quite bad. Ethernet comes up on my board and works fine after booting, but I can easily reproduce your problem by just doing ifconfig eth0 down /ifconfig eth0 up (which should call emac_dev_stop/emac_dev_open). # ifconfig eth0 up genirq: Flags mismatch irq 33. . So probably one of your start scripts (or dhcp or similar) does some ifconfig down/up, which then fails. I had a look at the code again, emac_dev_open requests the interrupts, whereas emac_dev_stop does not free them. If emac_dev_open is then called again by ifconfig eth0 up, it tries to request the interrupts again, resulting in the error above. I think the problem is a regression caused by Commit 6892b41d9701283085b655c6086fb57a5d63fa47 net: davinci: emac: Convert to devm_* api Signed-off-by: Lad, Prabhakar prabhakar.cse...@gmail.com This patch replaces request_irq() by devm_request_irq() in emac_dev_open and removes free_irq() from emac_dev_stop. But since emac_dev_open is called every time we do an ifconfig eth0 up, it tries to request the interrupts again and again and again (and fails). So I guess the correct solution would be to move the calls of devm_request_irq() to davinci_emac_probe(). I will send a patch to netdev, it solves the problem on my board. Christian -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH v2 RESEND 1/2] mtd: Add a retlen parameter to _get_{fact,user}_prot_info
Signed-off-by: Christian Riesch
Cc: Artem Bityutskiy
Cc: Brian Norris
---
drivers/mtd/chips/cfi_cmdset_0001.c | 31 +--
drivers/mtd/devices/mtd_dataflash.c |7 ---
drivers/mtd/mtdchar.c | 11 ++-
drivers/mtd/mtdcore.c | 12 ++--
drivers/mtd/mtdpart.c | 14 --
drivers/mtd/onenand/onenand_base.c | 30 --
include/linux/mtd/mtd.h | 16
7 files changed, 57 insertions(+), 64 deletions(-)
diff --git a/drivers/mtd/chips/cfi_cmdset_0001.c
b/drivers/mtd/chips/cfi_cmdset_0001.c
index 7751443..7aa581f 100644
--- a/drivers/mtd/chips/cfi_cmdset_0001.c
+++ b/drivers/mtd/chips/cfi_cmdset_0001.c
@@ -69,10 +69,10 @@ static int cfi_intelext_read_fact_prot_reg (struct mtd_info
*, loff_t, size_t, s
static int cfi_intelext_read_user_prot_reg (struct mtd_info *, loff_t, size_t,
size_t *, u_char *);
static int cfi_intelext_write_user_prot_reg (struct mtd_info *, loff_t,
size_t, size_t *, u_char *);
static int cfi_intelext_lock_user_prot_reg (struct mtd_info *, loff_t, size_t);
-static int cfi_intelext_get_fact_prot_info (struct mtd_info *,
- struct otp_info *, size_t);
-static int cfi_intelext_get_user_prot_info (struct mtd_info *,
- struct otp_info *, size_t);
+static int cfi_intelext_get_fact_prot_info(struct mtd_info *, size_t,
+ size_t *, struct otp_info *);
+static int cfi_intelext_get_user_prot_info(struct mtd_info *, size_t,
+ size_t *, struct otp_info *);
#endif
static int cfi_intelext_suspend (struct mtd_info *);
static void cfi_intelext_resume (struct mtd_info *);
@@ -2399,24 +2399,19 @@ static int cfi_intelext_lock_user_prot_reg(struct
mtd_info *mtd,
NULL, do_otp_lock, 1);
}
-static int cfi_intelext_get_fact_prot_info(struct mtd_info *mtd,
- struct otp_info *buf, size_t len)
-{
- size_t retlen;
- int ret;
+static int cfi_intelext_get_fact_prot_info(struct mtd_info *mtd, size_t len,
+ size_t *retlen, struct otp_info *buf)
- ret = cfi_intelext_otp_walk(mtd, 0, len, , (u_char *)buf, NULL,
0);
- return ret ? : retlen;
+{
+ return cfi_intelext_otp_walk(mtd, 0, len, retlen, (u_char *)buf,
+NULL, 0);
}
-static int cfi_intelext_get_user_prot_info(struct mtd_info *mtd,
- struct otp_info *buf, size_t len)
+static int cfi_intelext_get_user_prot_info(struct mtd_info *mtd, size_t len,
+ size_t *retlen, struct otp_info *buf)
{
- size_t retlen;
- int ret;
-
- ret = cfi_intelext_otp_walk(mtd, 0, len, , (u_char *)buf, NULL,
1);
- return ret ? : retlen;
+ return cfi_intelext_otp_walk(mtd, 0, len, retlen, (u_char *)buf,
+NULL, 1);
}
#endif
diff --git a/drivers/mtd/devices/mtd_dataflash.c
b/drivers/mtd/devices/mtd_dataflash.c
index 28779b6..09c69ce 100644
--- a/drivers/mtd/devices/mtd_dataflash.c
+++ b/drivers/mtd/devices/mtd_dataflash.c
@@ -442,8 +442,8 @@ static int dataflash_write(struct mtd_info *mtd, loff_t to,
size_t len,
#ifdef CONFIG_MTD_DATAFLASH_OTP
-static int dataflash_get_otp_info(struct mtd_info *mtd,
- struct otp_info *info, size_t len)
+static int dataflash_get_otp_info(struct mtd_info *mtd, size_t len,
+ size_t *retlen, struct otp_info *info)
{
/* Report both blocks as identical: bytes 0..64, locked.
* Unless the user block changed from all-ones, we can't
@@ -452,7 +452,8 @@ static int dataflash_get_otp_info(struct mtd_info *mtd,
info->start = 0;
info->length = 64;
info->locked = 1;
- return sizeof(*info);
+ *retlen = sizeof(*info);
+ return 0;
}
static ssize_t otp_read(struct spi_device *spi, unsigned base,
diff --git a/drivers/mtd/mtdchar.c b/drivers/mtd/mtdchar.c
index 684bfa3..0edb0ca 100644
--- a/drivers/mtd/mtdchar.c
+++ b/drivers/mtd/mtdchar.c
@@ -888,25 +888,26 @@ static int mtdchar_ioctl(struct file *file, u_int cmd,
u_long arg)
case OTPGETREGIONINFO:
{
struct otp_info *buf = kmalloc(4096, GFP_KERNEL);
+ size_t retlen;
if (!buf)
return -ENOMEM;
switch (mfi->mode) {
case MTD_FILE_MODE_OTP_FACTORY:
- ret = mtd_get_fact_prot_info(mtd, buf, 4096);
+ ret = mtd_get_fact_prot_info(mtd, 4096, , buf);
break;
case MTD_FILE_MODE_OTP_USER:
- ret = mtd_get_user_prot_info(mt
[PATCH v2 RESEND 2/2] mtd: Fix the behavior of otp write if there is not enough room for data
An OTP write shall write as much data as possible to the OTP memory
and return the number of bytes that have actually been written.
If no data could be written at all due to lack of OTP memory,
return -ENOSPC.
Signed-off-by: Christian Riesch
Cc: Artem Bityutskiy
Cc: Kyungmin Park
Cc: Amul Kumar Saha
---
drivers/mtd/chips/cfi_cmdset_0001.c | 13 +++--
drivers/mtd/devices/mtd_dataflash.c | 13 +
drivers/mtd/mtdchar.c |7 +++
drivers/mtd/onenand/onenand_base.c | 10 +-
4 files changed, 32 insertions(+), 11 deletions(-)
diff --git a/drivers/mtd/chips/cfi_cmdset_0001.c
b/drivers/mtd/chips/cfi_cmdset_0001.c
index 7aa581f..cf423a6 100644
--- a/drivers/mtd/chips/cfi_cmdset_0001.c
+++ b/drivers/mtd/chips/cfi_cmdset_0001.c
@@ -2387,8 +2387,17 @@ static int cfi_intelext_write_user_prot_reg(struct
mtd_info *mtd, loff_t from,
size_t len, size_t *retlen,
u_char *buf)
{
- return cfi_intelext_otp_walk(mtd, from, len, retlen,
-buf, do_otp_write, 1);
+ int ret;
+
+ ret = cfi_intelext_otp_walk(mtd, from, len, retlen,
+ buf, do_otp_write, 1);
+
+ /* if no data could be written due to lack of OTP memory,
+ return ENOSPC */
+ if (!ret && len && !(*retlen))
+ return -ENOSPC;
+
+ return ret;
}
static int cfi_intelext_lock_user_prot_reg(struct mtd_info *mtd,
diff --git a/drivers/mtd/devices/mtd_dataflash.c
b/drivers/mtd/devices/mtd_dataflash.c
index 09c69ce..5236d85 100644
--- a/drivers/mtd/devices/mtd_dataflash.c
+++ b/drivers/mtd/devices/mtd_dataflash.c
@@ -545,14 +545,11 @@ static int dataflash_write_user_otp(struct mtd_info *mtd,
struct dataflash*priv = mtd->priv;
int status;
- if (len > 64)
- return -EINVAL;
-
- /* Strictly speaking, we *could* truncate the write ... but
-* let's not do that for the only write that's ever possible.
-*/
- if ((from + len) > 64)
- return -EINVAL;
+ if ((from + len) > 64) {
+ len = 64 - from;
+ if (len <= 0)
+ return -ENOSPC;
+ }
/* OUT: OP_WRITE_SECURITY, 3 zeroes, 64 data-or-zero bytes
* IN: ignore all
diff --git a/drivers/mtd/mtdchar.c b/drivers/mtd/mtdchar.c
index 0edb0ca..db99031 100644
--- a/drivers/mtd/mtdchar.c
+++ b/drivers/mtd/mtdchar.c
@@ -323,6 +323,13 @@ static ssize_t mtdchar_write(struct file *file, const char
__user *buf, size_t c
default:
ret = mtd_write(mtd, *ppos, len, , kbuf);
}
+ /* return -ENOSPC only if no data was written */
+ if ((ret == -ENOSPC) && (total_retlen)) {
+ ret = 0;
+ retlen = 0;
+ /* drop the remaining data */
+ count = 0;
+ }
if (!ret) {
*ppos += retlen;
total_retlen += retlen;
diff --git a/drivers/mtd/onenand/onenand_base.c
b/drivers/mtd/onenand/onenand_base.c
index 419c538..6c49a6f 100644
--- a/drivers/mtd/onenand/onenand_base.c
+++ b/drivers/mtd/onenand/onenand_base.c
@@ -3316,7 +3316,15 @@ static int onenand_read_user_prot_reg(struct mtd_info
*mtd, loff_t from,
static int onenand_write_user_prot_reg(struct mtd_info *mtd, loff_t from,
size_t len, size_t *retlen, u_char *buf)
{
- return onenand_otp_walk(mtd, from, len, retlen, buf, do_otp_write,
MTD_OTP_USER);
+ int ret;
+ ret = onenand_otp_walk(mtd, from, len, retlen, buf, do_otp_write,
MTD_OTP_USER);
+
+ /* if no data could be written due to lack of OTP memory,
+ return ENOSPC */
+ if (!ret && len && !(*retlen))
+ return -ENOSPC;
+
+ return ret;
}
/**
--
1.7.9.5
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH v2 RESEND 0/2] mtd: Harmonize implementations of OTP write and _get_{fact,user}_prot_info
Hi all,
In the discussion on my patchset for the OTP support for
drivers/mtd/chips/cfi_cmdset_0002.c [1-5], Artem requested two changes in the
current code of the OTP write functions and the _get_{fact,user}_prot_info
code.
These two patches are an attempt to make the requested changes.
The first patch adds a retlen parameter to the _get_fact_prot_info and
_get_user_prot_info functions and thus harmonizes the implementation
with those of the write and read functions.
The second patch fixes a problem that I earlier addressed in [1]. After
the discussion about this patch on the mtd mailing list, I think that the
correct behavior of the write function should be the one specified in [6]:
Try to write as many bytes as possible and return the number of bytes
that were written. If no data could be written due to lack of OTP memory,
return -ENOSPC.
Artem, would you please have a look at these patches? I would like to know
if I understood you correctly, or if I missed something here. Please note
that I cannot test these patches since I do not have the hardware. The
patches are compile tested only. If these patches are ok, I will also
respin the OTP support patches for cfi_cmdset_0002.
Changes for v2:
- Fixed buggy cfi_intelext_get_fact_prot_info
Thank you!
Best regards,
Christian
[1] http://patchwork.ozlabs.org/patch/239897/
[2] http://patchwork.ozlabs.org/patch/240010/
[3] http://patchwork.ozlabs.org/patch/240007/
[4] http://patchwork.ozlabs.org/patch/240008/
[5] http://patchwork.ozlabs.org/patch/240009/
[6] http://pubs.opengroup.org/onlinepubs/9699919799/functions/write.html
Cc: Artem Bityutskiy
Cc: Kyungmin Park
Cc: Amul Kumar Saha
Cc: Brian Norris
Christian Riesch (2):
mtd: Add a retlen parameter to _get_{fact,user}_prot_info
mtd: Fix the behavior of otp write if there is not enough room for
data
drivers/mtd/chips/cfi_cmdset_0001.c | 44 +++
drivers/mtd/devices/mtd_dataflash.c | 20 +++-
drivers/mtd/mtdchar.c | 18 ++
drivers/mtd/mtdcore.c | 12 +-
drivers/mtd/mtdpart.c | 14 ++-
drivers/mtd/onenand/onenand_base.c | 40 ---
include/linux/mtd/mtd.h | 16 ++---
7 files changed, 89 insertions(+), 75 deletions(-)
--
1.7.9.5
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH v2 RESEND 2/2] mtd: Fix the behavior of otp write if there is not enough room for data
An OTP write shall write as much data as possible to the OTP memory
and return the number of bytes that have actually been written.
If no data could be written at all due to lack of OTP memory,
return -ENOSPC.
Signed-off-by: Christian Riesch christian.rie...@omicron.at
Cc: Artem Bityutskiy artem.bityuts...@linux.intel.com
Cc: Kyungmin Park kyungmin.p...@samsung.com
Cc: Amul Kumar Saha amul.s...@samsung.com
---
drivers/mtd/chips/cfi_cmdset_0001.c | 13 +++--
drivers/mtd/devices/mtd_dataflash.c | 13 +
drivers/mtd/mtdchar.c |7 +++
drivers/mtd/onenand/onenand_base.c | 10 +-
4 files changed, 32 insertions(+), 11 deletions(-)
diff --git a/drivers/mtd/chips/cfi_cmdset_0001.c
b/drivers/mtd/chips/cfi_cmdset_0001.c
index 7aa581f..cf423a6 100644
--- a/drivers/mtd/chips/cfi_cmdset_0001.c
+++ b/drivers/mtd/chips/cfi_cmdset_0001.c
@@ -2387,8 +2387,17 @@ static int cfi_intelext_write_user_prot_reg(struct
mtd_info *mtd, loff_t from,
size_t len, size_t *retlen,
u_char *buf)
{
- return cfi_intelext_otp_walk(mtd, from, len, retlen,
-buf, do_otp_write, 1);
+ int ret;
+
+ ret = cfi_intelext_otp_walk(mtd, from, len, retlen,
+ buf, do_otp_write, 1);
+
+ /* if no data could be written due to lack of OTP memory,
+ return ENOSPC */
+ if (!ret len !(*retlen))
+ return -ENOSPC;
+
+ return ret;
}
static int cfi_intelext_lock_user_prot_reg(struct mtd_info *mtd,
diff --git a/drivers/mtd/devices/mtd_dataflash.c
b/drivers/mtd/devices/mtd_dataflash.c
index 09c69ce..5236d85 100644
--- a/drivers/mtd/devices/mtd_dataflash.c
+++ b/drivers/mtd/devices/mtd_dataflash.c
@@ -545,14 +545,11 @@ static int dataflash_write_user_otp(struct mtd_info *mtd,
struct dataflash*priv = mtd-priv;
int status;
- if (len 64)
- return -EINVAL;
-
- /* Strictly speaking, we *could* truncate the write ... but
-* let's not do that for the only write that's ever possible.
-*/
- if ((from + len) 64)
- return -EINVAL;
+ if ((from + len) 64) {
+ len = 64 - from;
+ if (len = 0)
+ return -ENOSPC;
+ }
/* OUT: OP_WRITE_SECURITY, 3 zeroes, 64 data-or-zero bytes
* IN: ignore all
diff --git a/drivers/mtd/mtdchar.c b/drivers/mtd/mtdchar.c
index 0edb0ca..db99031 100644
--- a/drivers/mtd/mtdchar.c
+++ b/drivers/mtd/mtdchar.c
@@ -323,6 +323,13 @@ static ssize_t mtdchar_write(struct file *file, const char
__user *buf, size_t c
default:
ret = mtd_write(mtd, *ppos, len, retlen, kbuf);
}
+ /* return -ENOSPC only if no data was written */
+ if ((ret == -ENOSPC) (total_retlen)) {
+ ret = 0;
+ retlen = 0;
+ /* drop the remaining data */
+ count = 0;
+ }
if (!ret) {
*ppos += retlen;
total_retlen += retlen;
diff --git a/drivers/mtd/onenand/onenand_base.c
b/drivers/mtd/onenand/onenand_base.c
index 419c538..6c49a6f 100644
--- a/drivers/mtd/onenand/onenand_base.c
+++ b/drivers/mtd/onenand/onenand_base.c
@@ -3316,7 +3316,15 @@ static int onenand_read_user_prot_reg(struct mtd_info
*mtd, loff_t from,
static int onenand_write_user_prot_reg(struct mtd_info *mtd, loff_t from,
size_t len, size_t *retlen, u_char *buf)
{
- return onenand_otp_walk(mtd, from, len, retlen, buf, do_otp_write,
MTD_OTP_USER);
+ int ret;
+ ret = onenand_otp_walk(mtd, from, len, retlen, buf, do_otp_write,
MTD_OTP_USER);
+
+ /* if no data could be written due to lack of OTP memory,
+ return ENOSPC */
+ if (!ret len !(*retlen))
+ return -ENOSPC;
+
+ return ret;
}
/**
--
1.7.9.5
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH v2 RESEND 0/2] mtd: Harmonize implementations of OTP write and _get_{fact,user}_prot_info
Hi all,
In the discussion on my patchset for the OTP support for
drivers/mtd/chips/cfi_cmdset_0002.c [1-5], Artem requested two changes in the
current code of the OTP write functions and the _get_{fact,user}_prot_info
code.
These two patches are an attempt to make the requested changes.
The first patch adds a retlen parameter to the _get_fact_prot_info and
_get_user_prot_info functions and thus harmonizes the implementation
with those of the write and read functions.
The second patch fixes a problem that I earlier addressed in [1]. After
the discussion about this patch on the mtd mailing list, I think that the
correct behavior of the write function should be the one specified in [6]:
Try to write as many bytes as possible and return the number of bytes
that were written. If no data could be written due to lack of OTP memory,
return -ENOSPC.
Artem, would you please have a look at these patches? I would like to know
if I understood you correctly, or if I missed something here. Please note
that I cannot test these patches since I do not have the hardware. The
patches are compile tested only. If these patches are ok, I will also
respin the OTP support patches for cfi_cmdset_0002.
Changes for v2:
- Fixed buggy cfi_intelext_get_fact_prot_info
Thank you!
Best regards,
Christian
[1] http://patchwork.ozlabs.org/patch/239897/
[2] http://patchwork.ozlabs.org/patch/240010/
[3] http://patchwork.ozlabs.org/patch/240007/
[4] http://patchwork.ozlabs.org/patch/240008/
[5] http://patchwork.ozlabs.org/patch/240009/
[6] http://pubs.opengroup.org/onlinepubs/9699919799/functions/write.html
Cc: Artem Bityutskiy artem.bityuts...@linux.intel.com
Cc: Kyungmin Park kyungmin.p...@samsung.com
Cc: Amul Kumar Saha amul.s...@samsung.com
Cc: Brian Norris computersforpe...@gmail.com
Christian Riesch (2):
mtd: Add a retlen parameter to _get_{fact,user}_prot_info
mtd: Fix the behavior of otp write if there is not enough room for
data
drivers/mtd/chips/cfi_cmdset_0001.c | 44 +++
drivers/mtd/devices/mtd_dataflash.c | 20 +++-
drivers/mtd/mtdchar.c | 18 ++
drivers/mtd/mtdcore.c | 12 +-
drivers/mtd/mtdpart.c | 14 ++-
drivers/mtd/onenand/onenand_base.c | 40 ---
include/linux/mtd/mtd.h | 16 ++---
7 files changed, 89 insertions(+), 75 deletions(-)
--
1.7.9.5
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH v2 RESEND 1/2] mtd: Add a retlen parameter to _get_{fact,user}_prot_info
Signed-off-by: Christian Riesch christian.rie...@omicron.at
Cc: Artem Bityutskiy artem.bityuts...@linux.intel.com
Cc: Brian Norris computersforpe...@gmail.com
---
drivers/mtd/chips/cfi_cmdset_0001.c | 31 +--
drivers/mtd/devices/mtd_dataflash.c |7 ---
drivers/mtd/mtdchar.c | 11 ++-
drivers/mtd/mtdcore.c | 12 ++--
drivers/mtd/mtdpart.c | 14 --
drivers/mtd/onenand/onenand_base.c | 30 --
include/linux/mtd/mtd.h | 16
7 files changed, 57 insertions(+), 64 deletions(-)
diff --git a/drivers/mtd/chips/cfi_cmdset_0001.c
b/drivers/mtd/chips/cfi_cmdset_0001.c
index 7751443..7aa581f 100644
--- a/drivers/mtd/chips/cfi_cmdset_0001.c
+++ b/drivers/mtd/chips/cfi_cmdset_0001.c
@@ -69,10 +69,10 @@ static int cfi_intelext_read_fact_prot_reg (struct mtd_info
*, loff_t, size_t, s
static int cfi_intelext_read_user_prot_reg (struct mtd_info *, loff_t, size_t,
size_t *, u_char *);
static int cfi_intelext_write_user_prot_reg (struct mtd_info *, loff_t,
size_t, size_t *, u_char *);
static int cfi_intelext_lock_user_prot_reg (struct mtd_info *, loff_t, size_t);
-static int cfi_intelext_get_fact_prot_info (struct mtd_info *,
- struct otp_info *, size_t);
-static int cfi_intelext_get_user_prot_info (struct mtd_info *,
- struct otp_info *, size_t);
+static int cfi_intelext_get_fact_prot_info(struct mtd_info *, size_t,
+ size_t *, struct otp_info *);
+static int cfi_intelext_get_user_prot_info(struct mtd_info *, size_t,
+ size_t *, struct otp_info *);
#endif
static int cfi_intelext_suspend (struct mtd_info *);
static void cfi_intelext_resume (struct mtd_info *);
@@ -2399,24 +2399,19 @@ static int cfi_intelext_lock_user_prot_reg(struct
mtd_info *mtd,
NULL, do_otp_lock, 1);
}
-static int cfi_intelext_get_fact_prot_info(struct mtd_info *mtd,
- struct otp_info *buf, size_t len)
-{
- size_t retlen;
- int ret;
+static int cfi_intelext_get_fact_prot_info(struct mtd_info *mtd, size_t len,
+ size_t *retlen, struct otp_info *buf)
- ret = cfi_intelext_otp_walk(mtd, 0, len, retlen, (u_char *)buf, NULL,
0);
- return ret ? : retlen;
+{
+ return cfi_intelext_otp_walk(mtd, 0, len, retlen, (u_char *)buf,
+NULL, 0);
}
-static int cfi_intelext_get_user_prot_info(struct mtd_info *mtd,
- struct otp_info *buf, size_t len)
+static int cfi_intelext_get_user_prot_info(struct mtd_info *mtd, size_t len,
+ size_t *retlen, struct otp_info *buf)
{
- size_t retlen;
- int ret;
-
- ret = cfi_intelext_otp_walk(mtd, 0, len, retlen, (u_char *)buf, NULL,
1);
- return ret ? : retlen;
+ return cfi_intelext_otp_walk(mtd, 0, len, retlen, (u_char *)buf,
+NULL, 1);
}
#endif
diff --git a/drivers/mtd/devices/mtd_dataflash.c
b/drivers/mtd/devices/mtd_dataflash.c
index 28779b6..09c69ce 100644
--- a/drivers/mtd/devices/mtd_dataflash.c
+++ b/drivers/mtd/devices/mtd_dataflash.c
@@ -442,8 +442,8 @@ static int dataflash_write(struct mtd_info *mtd, loff_t to,
size_t len,
#ifdef CONFIG_MTD_DATAFLASH_OTP
-static int dataflash_get_otp_info(struct mtd_info *mtd,
- struct otp_info *info, size_t len)
+static int dataflash_get_otp_info(struct mtd_info *mtd, size_t len,
+ size_t *retlen, struct otp_info *info)
{
/* Report both blocks as identical: bytes 0..64, locked.
* Unless the user block changed from all-ones, we can't
@@ -452,7 +452,8 @@ static int dataflash_get_otp_info(struct mtd_info *mtd,
info-start = 0;
info-length = 64;
info-locked = 1;
- return sizeof(*info);
+ *retlen = sizeof(*info);
+ return 0;
}
static ssize_t otp_read(struct spi_device *spi, unsigned base,
diff --git a/drivers/mtd/mtdchar.c b/drivers/mtd/mtdchar.c
index 684bfa3..0edb0ca 100644
--- a/drivers/mtd/mtdchar.c
+++ b/drivers/mtd/mtdchar.c
@@ -888,25 +888,26 @@ static int mtdchar_ioctl(struct file *file, u_int cmd,
u_long arg)
case OTPGETREGIONINFO:
{
struct otp_info *buf = kmalloc(4096, GFP_KERNEL);
+ size_t retlen;
if (!buf)
return -ENOMEM;
switch (mfi-mode) {
case MTD_FILE_MODE_OTP_FACTORY:
- ret = mtd_get_fact_prot_info(mtd, buf, 4096);
+ ret = mtd_get_fact_prot_info(mtd, 4096, retlen, buf);
break;
case
[PATCH v2 2/2] mtd: Fix the behavior of otp write if there is not enough room for data
An OTP write shall write as much data as possible to the OTP memory
and return the number of bytes that have actually been written.
If no data could be written at all due to lack of OTP memory,
return -ENOSPC.
Signed-off-by: Christian Riesch
Cc: Artem Bityutskiy
Cc: Kyungmin Park
Cc: Amul Kumar Saha
---
drivers/mtd/chips/cfi_cmdset_0001.c | 13 +++--
drivers/mtd/devices/mtd_dataflash.c | 13 +
drivers/mtd/mtdchar.c |7 +++
drivers/mtd/onenand/onenand_base.c | 10 +-
4 files changed, 32 insertions(+), 11 deletions(-)
diff --git a/drivers/mtd/chips/cfi_cmdset_0001.c
b/drivers/mtd/chips/cfi_cmdset_0001.c
index 7aa581f..cf423a6 100644
--- a/drivers/mtd/chips/cfi_cmdset_0001.c
+++ b/drivers/mtd/chips/cfi_cmdset_0001.c
@@ -2387,8 +2387,17 @@ static int cfi_intelext_write_user_prot_reg(struct
mtd_info *mtd, loff_t from,
size_t len, size_t *retlen,
u_char *buf)
{
- return cfi_intelext_otp_walk(mtd, from, len, retlen,
-buf, do_otp_write, 1);
+ int ret;
+
+ ret = cfi_intelext_otp_walk(mtd, from, len, retlen,
+ buf, do_otp_write, 1);
+
+ /* if no data could be written due to lack of OTP memory,
+ return ENOSPC */
+ if (!ret && len && !(*retlen))
+ return -ENOSPC;
+
+ return ret;
}
static int cfi_intelext_lock_user_prot_reg(struct mtd_info *mtd,
diff --git a/drivers/mtd/devices/mtd_dataflash.c
b/drivers/mtd/devices/mtd_dataflash.c
index 09c69ce..5236d85 100644
--- a/drivers/mtd/devices/mtd_dataflash.c
+++ b/drivers/mtd/devices/mtd_dataflash.c
@@ -545,14 +545,11 @@ static int dataflash_write_user_otp(struct mtd_info *mtd,
struct dataflash*priv = mtd->priv;
int status;
- if (len > 64)
- return -EINVAL;
-
- /* Strictly speaking, we *could* truncate the write ... but
-* let's not do that for the only write that's ever possible.
-*/
- if ((from + len) > 64)
- return -EINVAL;
+ if ((from + len) > 64) {
+ len = 64 - from;
+ if (len <= 0)
+ return -ENOSPC;
+ }
/* OUT: OP_WRITE_SECURITY, 3 zeroes, 64 data-or-zero bytes
* IN: ignore all
diff --git a/drivers/mtd/mtdchar.c b/drivers/mtd/mtdchar.c
index 0edb0ca..db99031 100644
--- a/drivers/mtd/mtdchar.c
+++ b/drivers/mtd/mtdchar.c
@@ -323,6 +323,13 @@ static ssize_t mtdchar_write(struct file *file, const char
__user *buf, size_t c
default:
ret = mtd_write(mtd, *ppos, len, , kbuf);
}
+ /* return -ENOSPC only if no data was written */
+ if ((ret == -ENOSPC) && (total_retlen)) {
+ ret = 0;
+ retlen = 0;
+ /* drop the remaining data */
+ count = 0;
+ }
if (!ret) {
*ppos += retlen;
total_retlen += retlen;
diff --git a/drivers/mtd/onenand/onenand_base.c
b/drivers/mtd/onenand/onenand_base.c
index 419c538..6c49a6f 100644
--- a/drivers/mtd/onenand/onenand_base.c
+++ b/drivers/mtd/onenand/onenand_base.c
@@ -3316,7 +3316,15 @@ static int onenand_read_user_prot_reg(struct mtd_info
*mtd, loff_t from,
static int onenand_write_user_prot_reg(struct mtd_info *mtd, loff_t from,
size_t len, size_t *retlen, u_char *buf)
{
- return onenand_otp_walk(mtd, from, len, retlen, buf, do_otp_write,
MTD_OTP_USER);
+ int ret;
+ ret = onenand_otp_walk(mtd, from, len, retlen, buf, do_otp_write,
MTD_OTP_USER);
+
+ /* if no data could be written due to lack of OTP memory,
+ return ENOSPC */
+ if (!ret && len && !(*retlen))
+ return -ENOSPC;
+
+ return ret;
}
/**
--
1.7.9.5
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH v2 0/2] mtd: Harmonize implementations of OTP write and _get_{fact,user}_prot_info
Hi all,
In the discussion on my patchset for the OTP support for
drivers/mtd/chips/cfi_cmdset_0002.c [1-5], Artem requested two changes in the
current code of the OTP write functions and the _get_{fact,user}_prot_info
code.
These two patches are an attempt to make the requested changes.
The first patch adds a retlen parameter to the _get_fact_prot_info and
_get_user_prot_info functions and thus harmonizes the implementation
with those of the write and read functions.
The second patch fixes a problem that I earlier addressed in [1]. After
the discussion about this patch on the mtd mailing list, I think that the
correct behavior of the write function should be the one specified in [6]:
Try to write as many bytes as possible and return the number of bytes
that were written. If no data could be written due to lack of OTP memory,
return -ENOSPC.
Artem, would you please have a look at these patches? I would like to know
if I understood you correctly, or if I missed something here. Please note
that I cannot test these patches since I do not have the hardware. The
patches are compile tested only. If these patches are ok, I will also
respin the OTP support patches for cfi_cmdset_0002.
Changes for v2:
- Fixed buggy cfi_intelext_get_fact_prot_info
Thank you!
Best regards,
Christian
[1] http://patchwork.ozlabs.org/patch/239897/
[2] http://patchwork.ozlabs.org/patch/240010/
[3] http://patchwork.ozlabs.org/patch/240007/
[4] http://patchwork.ozlabs.org/patch/240008/
[5] http://patchwork.ozlabs.org/patch/240009/
[6] http://pubs.opengroup.org/onlinepubs/9699919799/functions/write.html
Cc: Artem Bityutskiy
Cc: Kyungmin Park
Cc: Amul Kumar Saha
Cc: Brian Norris
Christian Riesch (2):
mtd: Add a retlen parameter to _get_{fact,user}_prot_info
mtd: Fix the behavior of otp write if there is not enough room for
data
drivers/mtd/chips/cfi_cmdset_0001.c | 44 +++
drivers/mtd/devices/mtd_dataflash.c | 20 +++-
drivers/mtd/mtdchar.c | 18 ++
drivers/mtd/mtdcore.c | 12 +-
drivers/mtd/mtdpart.c | 14 ++-
drivers/mtd/onenand/onenand_base.c | 40 ---
include/linux/mtd/mtd.h | 16 ++---
7 files changed, 89 insertions(+), 75 deletions(-)
--
1.7.9.5
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH v2 1/2] mtd: Add a retlen parameter to _get_{fact,user}_prot_info
Signed-off-by: Christian Riesch
Cc: Artem Bityutskiy
Cc: Brian Norris
---
drivers/mtd/chips/cfi_cmdset_0001.c | 31 +--
drivers/mtd/devices/mtd_dataflash.c |7 ---
drivers/mtd/mtdchar.c | 11 ++-
drivers/mtd/mtdcore.c | 12 ++--
drivers/mtd/mtdpart.c | 14 --
drivers/mtd/onenand/onenand_base.c | 30 --
include/linux/mtd/mtd.h | 16
7 files changed, 57 insertions(+), 64 deletions(-)
diff --git a/drivers/mtd/chips/cfi_cmdset_0001.c
b/drivers/mtd/chips/cfi_cmdset_0001.c
index 7751443..7aa581f 100644
--- a/drivers/mtd/chips/cfi_cmdset_0001.c
+++ b/drivers/mtd/chips/cfi_cmdset_0001.c
@@ -69,10 +69,10 @@ static int cfi_intelext_read_fact_prot_reg (struct mtd_info
*, loff_t, size_t, s
static int cfi_intelext_read_user_prot_reg (struct mtd_info *, loff_t, size_t,
size_t *, u_char *);
static int cfi_intelext_write_user_prot_reg (struct mtd_info *, loff_t,
size_t, size_t *, u_char *);
static int cfi_intelext_lock_user_prot_reg (struct mtd_info *, loff_t, size_t);
-static int cfi_intelext_get_fact_prot_info (struct mtd_info *,
- struct otp_info *, size_t);
-static int cfi_intelext_get_user_prot_info (struct mtd_info *,
- struct otp_info *, size_t);
+static int cfi_intelext_get_fact_prot_info(struct mtd_info *, size_t,
+ size_t *, struct otp_info *);
+static int cfi_intelext_get_user_prot_info(struct mtd_info *, size_t,
+ size_t *, struct otp_info *);
#endif
static int cfi_intelext_suspend (struct mtd_info *);
static void cfi_intelext_resume (struct mtd_info *);
@@ -2399,24 +2399,19 @@ static int cfi_intelext_lock_user_prot_reg(struct
mtd_info *mtd,
NULL, do_otp_lock, 1);
}
-static int cfi_intelext_get_fact_prot_info(struct mtd_info *mtd,
- struct otp_info *buf, size_t len)
-{
- size_t retlen;
- int ret;
+static int cfi_intelext_get_fact_prot_info(struct mtd_info *mtd, size_t len,
+ size_t *retlen, struct otp_info *buf)
- ret = cfi_intelext_otp_walk(mtd, 0, len, , (u_char *)buf, NULL,
0);
- return ret ? : retlen;
+{
+ return cfi_intelext_otp_walk(mtd, 0, len, retlen, (u_char *)buf,
+NULL, 0);
}
-static int cfi_intelext_get_user_prot_info(struct mtd_info *mtd,
- struct otp_info *buf, size_t len)
+static int cfi_intelext_get_user_prot_info(struct mtd_info *mtd, size_t len,
+ size_t *retlen, struct otp_info *buf)
{
- size_t retlen;
- int ret;
-
- ret = cfi_intelext_otp_walk(mtd, 0, len, , (u_char *)buf, NULL,
1);
- return ret ? : retlen;
+ return cfi_intelext_otp_walk(mtd, 0, len, retlen, (u_char *)buf,
+NULL, 1);
}
#endif
diff --git a/drivers/mtd/devices/mtd_dataflash.c
b/drivers/mtd/devices/mtd_dataflash.c
index 28779b6..09c69ce 100644
--- a/drivers/mtd/devices/mtd_dataflash.c
+++ b/drivers/mtd/devices/mtd_dataflash.c
@@ -442,8 +442,8 @@ static int dataflash_write(struct mtd_info *mtd, loff_t to,
size_t len,
#ifdef CONFIG_MTD_DATAFLASH_OTP
-static int dataflash_get_otp_info(struct mtd_info *mtd,
- struct otp_info *info, size_t len)
+static int dataflash_get_otp_info(struct mtd_info *mtd, size_t len,
+ size_t *retlen, struct otp_info *info)
{
/* Report both blocks as identical: bytes 0..64, locked.
* Unless the user block changed from all-ones, we can't
@@ -452,7 +452,8 @@ static int dataflash_get_otp_info(struct mtd_info *mtd,
info->start = 0;
info->length = 64;
info->locked = 1;
- return sizeof(*info);
+ *retlen = sizeof(*info);
+ return 0;
}
static ssize_t otp_read(struct spi_device *spi, unsigned base,
diff --git a/drivers/mtd/mtdchar.c b/drivers/mtd/mtdchar.c
index 684bfa3..0edb0ca 100644
--- a/drivers/mtd/mtdchar.c
+++ b/drivers/mtd/mtdchar.c
@@ -888,25 +888,26 @@ static int mtdchar_ioctl(struct file *file, u_int cmd,
u_long arg)
case OTPGETREGIONINFO:
{
struct otp_info *buf = kmalloc(4096, GFP_KERNEL);
+ size_t retlen;
if (!buf)
return -ENOMEM;
switch (mfi->mode) {
case MTD_FILE_MODE_OTP_FACTORY:
- ret = mtd_get_fact_prot_info(mtd, buf, 4096);
+ ret = mtd_get_fact_prot_info(mtd, 4096, , buf);
break;
case MTD_FILE_MODE_OTP_USER:
- ret = mtd_get_user_prot_info(mt
[PATCH v2 1/2] mtd: Add a retlen parameter to _get_{fact,user}_prot_info
Signed-off-by: Christian Riesch christian.rie...@omicron.at
Cc: Artem Bityutskiy artem.bityuts...@linux.intel.com
Cc: Brian Norris computersforpe...@gmail.com
---
drivers/mtd/chips/cfi_cmdset_0001.c | 31 +--
drivers/mtd/devices/mtd_dataflash.c |7 ---
drivers/mtd/mtdchar.c | 11 ++-
drivers/mtd/mtdcore.c | 12 ++--
drivers/mtd/mtdpart.c | 14 --
drivers/mtd/onenand/onenand_base.c | 30 --
include/linux/mtd/mtd.h | 16
7 files changed, 57 insertions(+), 64 deletions(-)
diff --git a/drivers/mtd/chips/cfi_cmdset_0001.c
b/drivers/mtd/chips/cfi_cmdset_0001.c
index 7751443..7aa581f 100644
--- a/drivers/mtd/chips/cfi_cmdset_0001.c
+++ b/drivers/mtd/chips/cfi_cmdset_0001.c
@@ -69,10 +69,10 @@ static int cfi_intelext_read_fact_prot_reg (struct mtd_info
*, loff_t, size_t, s
static int cfi_intelext_read_user_prot_reg (struct mtd_info *, loff_t, size_t,
size_t *, u_char *);
static int cfi_intelext_write_user_prot_reg (struct mtd_info *, loff_t,
size_t, size_t *, u_char *);
static int cfi_intelext_lock_user_prot_reg (struct mtd_info *, loff_t, size_t);
-static int cfi_intelext_get_fact_prot_info (struct mtd_info *,
- struct otp_info *, size_t);
-static int cfi_intelext_get_user_prot_info (struct mtd_info *,
- struct otp_info *, size_t);
+static int cfi_intelext_get_fact_prot_info(struct mtd_info *, size_t,
+ size_t *, struct otp_info *);
+static int cfi_intelext_get_user_prot_info(struct mtd_info *, size_t,
+ size_t *, struct otp_info *);
#endif
static int cfi_intelext_suspend (struct mtd_info *);
static void cfi_intelext_resume (struct mtd_info *);
@@ -2399,24 +2399,19 @@ static int cfi_intelext_lock_user_prot_reg(struct
mtd_info *mtd,
NULL, do_otp_lock, 1);
}
-static int cfi_intelext_get_fact_prot_info(struct mtd_info *mtd,
- struct otp_info *buf, size_t len)
-{
- size_t retlen;
- int ret;
+static int cfi_intelext_get_fact_prot_info(struct mtd_info *mtd, size_t len,
+ size_t *retlen, struct otp_info *buf)
- ret = cfi_intelext_otp_walk(mtd, 0, len, retlen, (u_char *)buf, NULL,
0);
- return ret ? : retlen;
+{
+ return cfi_intelext_otp_walk(mtd, 0, len, retlen, (u_char *)buf,
+NULL, 0);
}
-static int cfi_intelext_get_user_prot_info(struct mtd_info *mtd,
- struct otp_info *buf, size_t len)
+static int cfi_intelext_get_user_prot_info(struct mtd_info *mtd, size_t len,
+ size_t *retlen, struct otp_info *buf)
{
- size_t retlen;
- int ret;
-
- ret = cfi_intelext_otp_walk(mtd, 0, len, retlen, (u_char *)buf, NULL,
1);
- return ret ? : retlen;
+ return cfi_intelext_otp_walk(mtd, 0, len, retlen, (u_char *)buf,
+NULL, 1);
}
#endif
diff --git a/drivers/mtd/devices/mtd_dataflash.c
b/drivers/mtd/devices/mtd_dataflash.c
index 28779b6..09c69ce 100644
--- a/drivers/mtd/devices/mtd_dataflash.c
+++ b/drivers/mtd/devices/mtd_dataflash.c
@@ -442,8 +442,8 @@ static int dataflash_write(struct mtd_info *mtd, loff_t to,
size_t len,
#ifdef CONFIG_MTD_DATAFLASH_OTP
-static int dataflash_get_otp_info(struct mtd_info *mtd,
- struct otp_info *info, size_t len)
+static int dataflash_get_otp_info(struct mtd_info *mtd, size_t len,
+ size_t *retlen, struct otp_info *info)
{
/* Report both blocks as identical: bytes 0..64, locked.
* Unless the user block changed from all-ones, we can't
@@ -452,7 +452,8 @@ static int dataflash_get_otp_info(struct mtd_info *mtd,
info-start = 0;
info-length = 64;
info-locked = 1;
- return sizeof(*info);
+ *retlen = sizeof(*info);
+ return 0;
}
static ssize_t otp_read(struct spi_device *spi, unsigned base,
diff --git a/drivers/mtd/mtdchar.c b/drivers/mtd/mtdchar.c
index 684bfa3..0edb0ca 100644
--- a/drivers/mtd/mtdchar.c
+++ b/drivers/mtd/mtdchar.c
@@ -888,25 +888,26 @@ static int mtdchar_ioctl(struct file *file, u_int cmd,
u_long arg)
case OTPGETREGIONINFO:
{
struct otp_info *buf = kmalloc(4096, GFP_KERNEL);
+ size_t retlen;
if (!buf)
return -ENOMEM;
switch (mfi-mode) {
case MTD_FILE_MODE_OTP_FACTORY:
- ret = mtd_get_fact_prot_info(mtd, buf, 4096);
+ ret = mtd_get_fact_prot_info(mtd, 4096, retlen, buf);
break;
case
[PATCH v2 2/2] mtd: Fix the behavior of otp write if there is not enough room for data
An OTP write shall write as much data as possible to the OTP memory
and return the number of bytes that have actually been written.
If no data could be written at all due to lack of OTP memory,
return -ENOSPC.
Signed-off-by: Christian Riesch christian.rie...@omicron.at
Cc: Artem Bityutskiy artem.bityuts...@linux.intel.com
Cc: Kyungmin Park kyungmin.p...@samsung.com
Cc: Amul Kumar Saha amul.s...@samsung.com
---
drivers/mtd/chips/cfi_cmdset_0001.c | 13 +++--
drivers/mtd/devices/mtd_dataflash.c | 13 +
drivers/mtd/mtdchar.c |7 +++
drivers/mtd/onenand/onenand_base.c | 10 +-
4 files changed, 32 insertions(+), 11 deletions(-)
diff --git a/drivers/mtd/chips/cfi_cmdset_0001.c
b/drivers/mtd/chips/cfi_cmdset_0001.c
index 7aa581f..cf423a6 100644
--- a/drivers/mtd/chips/cfi_cmdset_0001.c
+++ b/drivers/mtd/chips/cfi_cmdset_0001.c
@@ -2387,8 +2387,17 @@ static int cfi_intelext_write_user_prot_reg(struct
mtd_info *mtd, loff_t from,
size_t len, size_t *retlen,
u_char *buf)
{
- return cfi_intelext_otp_walk(mtd, from, len, retlen,
-buf, do_otp_write, 1);
+ int ret;
+
+ ret = cfi_intelext_otp_walk(mtd, from, len, retlen,
+ buf, do_otp_write, 1);
+
+ /* if no data could be written due to lack of OTP memory,
+ return ENOSPC */
+ if (!ret len !(*retlen))
+ return -ENOSPC;
+
+ return ret;
}
static int cfi_intelext_lock_user_prot_reg(struct mtd_info *mtd,
diff --git a/drivers/mtd/devices/mtd_dataflash.c
b/drivers/mtd/devices/mtd_dataflash.c
index 09c69ce..5236d85 100644
--- a/drivers/mtd/devices/mtd_dataflash.c
+++ b/drivers/mtd/devices/mtd_dataflash.c
@@ -545,14 +545,11 @@ static int dataflash_write_user_otp(struct mtd_info *mtd,
struct dataflash*priv = mtd-priv;
int status;
- if (len 64)
- return -EINVAL;
-
- /* Strictly speaking, we *could* truncate the write ... but
-* let's not do that for the only write that's ever possible.
-*/
- if ((from + len) 64)
- return -EINVAL;
+ if ((from + len) 64) {
+ len = 64 - from;
+ if (len = 0)
+ return -ENOSPC;
+ }
/* OUT: OP_WRITE_SECURITY, 3 zeroes, 64 data-or-zero bytes
* IN: ignore all
diff --git a/drivers/mtd/mtdchar.c b/drivers/mtd/mtdchar.c
index 0edb0ca..db99031 100644
--- a/drivers/mtd/mtdchar.c
+++ b/drivers/mtd/mtdchar.c
@@ -323,6 +323,13 @@ static ssize_t mtdchar_write(struct file *file, const char
__user *buf, size_t c
default:
ret = mtd_write(mtd, *ppos, len, retlen, kbuf);
}
+ /* return -ENOSPC only if no data was written */
+ if ((ret == -ENOSPC) (total_retlen)) {
+ ret = 0;
+ retlen = 0;
+ /* drop the remaining data */
+ count = 0;
+ }
if (!ret) {
*ppos += retlen;
total_retlen += retlen;
diff --git a/drivers/mtd/onenand/onenand_base.c
b/drivers/mtd/onenand/onenand_base.c
index 419c538..6c49a6f 100644
--- a/drivers/mtd/onenand/onenand_base.c
+++ b/drivers/mtd/onenand/onenand_base.c
@@ -3316,7 +3316,15 @@ static int onenand_read_user_prot_reg(struct mtd_info
*mtd, loff_t from,
static int onenand_write_user_prot_reg(struct mtd_info *mtd, loff_t from,
size_t len, size_t *retlen, u_char *buf)
{
- return onenand_otp_walk(mtd, from, len, retlen, buf, do_otp_write,
MTD_OTP_USER);
+ int ret;
+ ret = onenand_otp_walk(mtd, from, len, retlen, buf, do_otp_write,
MTD_OTP_USER);
+
+ /* if no data could be written due to lack of OTP memory,
+ return ENOSPC */
+ if (!ret len !(*retlen))
+ return -ENOSPC;
+
+ return ret;
}
/**
--
1.7.9.5
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH v2 0/2] mtd: Harmonize implementations of OTP write and _get_{fact,user}_prot_info
Hi all,
In the discussion on my patchset for the OTP support for
drivers/mtd/chips/cfi_cmdset_0002.c [1-5], Artem requested two changes in the
current code of the OTP write functions and the _get_{fact,user}_prot_info
code.
These two patches are an attempt to make the requested changes.
The first patch adds a retlen parameter to the _get_fact_prot_info and
_get_user_prot_info functions and thus harmonizes the implementation
with those of the write and read functions.
The second patch fixes a problem that I earlier addressed in [1]. After
the discussion about this patch on the mtd mailing list, I think that the
correct behavior of the write function should be the one specified in [6]:
Try to write as many bytes as possible and return the number of bytes
that were written. If no data could be written due to lack of OTP memory,
return -ENOSPC.
Artem, would you please have a look at these patches? I would like to know
if I understood you correctly, or if I missed something here. Please note
that I cannot test these patches since I do not have the hardware. The
patches are compile tested only. If these patches are ok, I will also
respin the OTP support patches for cfi_cmdset_0002.
Changes for v2:
- Fixed buggy cfi_intelext_get_fact_prot_info
Thank you!
Best regards,
Christian
[1] http://patchwork.ozlabs.org/patch/239897/
[2] http://patchwork.ozlabs.org/patch/240010/
[3] http://patchwork.ozlabs.org/patch/240007/
[4] http://patchwork.ozlabs.org/patch/240008/
[5] http://patchwork.ozlabs.org/patch/240009/
[6] http://pubs.opengroup.org/onlinepubs/9699919799/functions/write.html
Cc: Artem Bityutskiy artem.bityuts...@linux.intel.com
Cc: Kyungmin Park kyungmin.p...@samsung.com
Cc: Amul Kumar Saha amul.s...@samsung.com
Cc: Brian Norris computersforpe...@gmail.com
Christian Riesch (2):
mtd: Add a retlen parameter to _get_{fact,user}_prot_info
mtd: Fix the behavior of otp write if there is not enough room for
data
drivers/mtd/chips/cfi_cmdset_0001.c | 44 +++
drivers/mtd/devices/mtd_dataflash.c | 20 +++-
drivers/mtd/mtdchar.c | 18 ++
drivers/mtd/mtdcore.c | 12 +-
drivers/mtd/mtdpart.c | 14 ++-
drivers/mtd/onenand/onenand_base.c | 40 ---
include/linux/mtd/mtd.h | 16 ++---
7 files changed, 89 insertions(+), 75 deletions(-)
--
1.7.9.5
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ax88179_178a: ASIX AX88179_178A USB 3.0/2.0 to gigabit ethernet adapter driver
Hi,
On 2013-01-11 03:45, Ming Lei wrote:
Cc netdev and usb lists.
On Fri, Jan 11, 2013 at 9:17 AM, wrote:
From: Freddy Xin
This patch adds a driver for ASIX's AX88179 family of USB 3.0/2.0
to gigabit ethernet adapters. It's based on the AX88xxx driver but
the usb commands used to access registers for AX88179 are completely different.
This driver had been verified on x86 system with AX88179/AX88178A and
Sitcomm LN-032 USB dongles.
Signed-off-by: Freddy Xin
---
drivers/net/usb/Kconfig| 18 +
drivers/net/usb/Makefile |1 +
drivers/net/usb/ax88179_178a.c | 1457
3 files changed, 1476 insertions(+)
create mode 100644 drivers/net/usb/ax88179_178a.c
[...]
diff --git a/drivers/net/usb/ax88179_178a.c b/drivers/net/usb/ax88179_178a.c
new file mode 100644
index 000..47504ea
--- /dev/null
+++ b/drivers/net/usb/ax88179_178a.c
[...]
+static int ax88179_read_cmd(struct usbnet *dev, u8 cmd, u16 value, u16 index,
+ u16 size, void *data)
+{
+ int ret;
+ ret = usbnet_read_cmd(dev, cmd,
+ USB_DIR_IN | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
+ value, index, data, size);
+
+ if (ret != size && ret >= 0)
+ return -EINVAL;
+ return ret;
+}
+
+static int ax88179_write_cmd(struct usbnet *dev, u8 cmd, u16 value, u16 index,
+u16 size, void *data)
+{
+ return usbnet_write_cmd(dev, cmd, USB_DIR_OUT | USB_TYPE_VENDOR
+ | USB_RECIP_DEVICE, value, index, data, size);
+}
+
+static void
+ax88179_write_cmd_async(struct usbnet *dev, u8 cmd, u16 value, u16 index,
+ u16 size, void *data)
+{
+ usbnet_write_cmd_async(dev, cmd,
+ USB_DIR_OUT | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
+ value, index, data, size);
+}
+
ax88179_read_cmd, ax88179_write_cmd, and ax88179_write_cmd_async
duplicate code from asix_common.c.
[...]
+static int ax88179_get_eeprom(struct net_device *net,
+ struct ethtool_eeprom *eeprom, u8 *data)
+{
+ struct usbnet *dev = netdev_priv(net);
+ u16 *ebuf = (u16 *)data;
+ int i;
+
+ /* Crude hack to ensure that we don't overwrite memory
+* if an odd length is supplied
Have a look at asix_get_eeprom() in asix_common.c for a better
implementation. There is also code for programming the eeprom
(asix_set_eeprom in asix_common.c), can this be used for the
AX88179/178A as well?
Regards, Christian
+*/
+ if (eeprom->len % 2)
+ return -EINVAL;
+
+ /* ax8817x returns 2 bytes from eeprom on read */
+ for (i = 0; i < eeprom->len / 2; i++) {
+ if (ax88179_read_cmd(dev, AX_ACCESS_EEPROM,
+ eeprom->offset + i, 1, 2, [i]) < 0)
+ return -EINVAL;
+ }
+ return 0;
+}
+
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ax88179_178a: ASIX AX88179_178A USB 3.0/2.0 to gigabit ethernet adapter driver
Hi,
On 2013-01-11 03:45, Ming Lei wrote:
Cc netdev and usb lists.
On Fri, Jan 11, 2013 at 9:17 AM, fre...@asix.com.tw wrote:
From: Freddy Xin fre...@asix.com.tw
This patch adds a driver for ASIX's AX88179 family of USB 3.0/2.0
to gigabit ethernet adapters. It's based on the AX88xxx driver but
the usb commands used to access registers for AX88179 are completely different.
This driver had been verified on x86 system with AX88179/AX88178A and
Sitcomm LN-032 USB dongles.
Signed-off-by: Freddy Xin fre...@asix.com.tw
---
drivers/net/usb/Kconfig| 18 +
drivers/net/usb/Makefile |1 +
drivers/net/usb/ax88179_178a.c | 1457
3 files changed, 1476 insertions(+)
create mode 100644 drivers/net/usb/ax88179_178a.c
[...]
diff --git a/drivers/net/usb/ax88179_178a.c b/drivers/net/usb/ax88179_178a.c
new file mode 100644
index 000..47504ea
--- /dev/null
+++ b/drivers/net/usb/ax88179_178a.c
[...]
+static int ax88179_read_cmd(struct usbnet *dev, u8 cmd, u16 value, u16 index,
+ u16 size, void *data)
+{
+ int ret;
+ ret = usbnet_read_cmd(dev, cmd,
+ USB_DIR_IN | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
+ value, index, data, size);
+
+ if (ret != size ret = 0)
+ return -EINVAL;
+ return ret;
+}
+
+static int ax88179_write_cmd(struct usbnet *dev, u8 cmd, u16 value, u16 index,
+u16 size, void *data)
+{
+ return usbnet_write_cmd(dev, cmd, USB_DIR_OUT | USB_TYPE_VENDOR
+ | USB_RECIP_DEVICE, value, index, data, size);
+}
+
+static void
+ax88179_write_cmd_async(struct usbnet *dev, u8 cmd, u16 value, u16 index,
+ u16 size, void *data)
+{
+ usbnet_write_cmd_async(dev, cmd,
+ USB_DIR_OUT | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
+ value, index, data, size);
+}
+
ax88179_read_cmd, ax88179_write_cmd, and ax88179_write_cmd_async
duplicate code from asix_common.c.
[...]
+static int ax88179_get_eeprom(struct net_device *net,
+ struct ethtool_eeprom *eeprom, u8 *data)
+{
+ struct usbnet *dev = netdev_priv(net);
+ u16 *ebuf = (u16 *)data;
+ int i;
+
+ /* Crude hack to ensure that we don't overwrite memory
+* if an odd length is supplied
Have a look at asix_get_eeprom() in asix_common.c for a better
implementation. There is also code for programming the eeprom
(asix_set_eeprom in asix_common.c), can this be used for the
AX88179/178A as well?
Regards, Christian
+*/
+ if (eeprom-len % 2)
+ return -EINVAL;
+
+ /* ax8817x returns 2 bytes from eeprom on read */
+ for (i = 0; i eeprom-len / 2; i++) {
+ if (ax88179_read_cmd(dev, AX_ACCESS_EEPROM,
+ eeprom-offset + i, 1, 2, ebuf[i]) 0)
+ return -EINVAL;
+ }
+ return 0;
+}
+
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: linux-next: build failure after merge of the net-next tree
[Sent again due to problems with email client]
Hi,
On 07/20/2012 04:01 AM, Stephen Rothwell wrote:
Hi all,
After merging the net-next tree, today's linux-next build (powerpc
pmac32_defconfig) failed like this:
ERROR: "phy_disconnect" [drivers/net/usb/asix.ko] undefined!
ERROR: "phy_stop" [drivers/net/usb/asix.ko] undefined!
ERROR: "phy_ethtool_gset" [drivers/net/usb/asix.ko] undefined!
ERROR: "mdiobus_unregister" [drivers/net/usb/asix.ko] undefined!
ERROR: "phy_start_aneg" [drivers/net/usb/asix.ko] undefined!
ERROR: "phy_print_status" [drivers/net/usb/asix.ko] undefined!
ERROR: "phy_start" [drivers/net/usb/asix.ko] undefined!
ERROR: "mdiobus_free" [drivers/net/usb/asix.ko] undefined!
ERROR: "mdiobus_register" [drivers/net/usb/asix.ko] undefined!
ERROR: "genphy_resume" [drivers/net/usb/asix.ko] undefined!
ERROR: "phy_connect" [drivers/net/usb/asix.ko] undefined!
ERROR: "phy_mii_ioctl" [drivers/net/usb/asix.ko] undefined!
ERROR: "phy_ethtool_sset" [drivers/net/usb/asix.ko] undefined!
ERROR: "mdiobus_alloc_size" [drivers/net/usb/asix.ko] undefined!
Caused by commit 16626b0cc3d5 ("asix: Add a new driver for the AX88172A")
and reverting that commit fixes the build.
Sorry about that. I missed the dependency of the new driver on phylib. A
fix for this problem is already in net-next, see commit 215029375c83.
Thanks, Christian
Thanks to Mikey for reporting this porblem.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: linux-next: build failure after merge of the net-next tree
[Sent again due to problems with email client] Hi, On 07/20/2012 04:01 AM, Stephen Rothwell wrote: Hi all, After merging the net-next tree, today's linux-next build (powerpc pmac32_defconfig) failed like this: ERROR: phy_disconnect [drivers/net/usb/asix.ko] undefined! ERROR: phy_stop [drivers/net/usb/asix.ko] undefined! ERROR: phy_ethtool_gset [drivers/net/usb/asix.ko] undefined! ERROR: mdiobus_unregister [drivers/net/usb/asix.ko] undefined! ERROR: phy_start_aneg [drivers/net/usb/asix.ko] undefined! ERROR: phy_print_status [drivers/net/usb/asix.ko] undefined! ERROR: phy_start [drivers/net/usb/asix.ko] undefined! ERROR: mdiobus_free [drivers/net/usb/asix.ko] undefined! ERROR: mdiobus_register [drivers/net/usb/asix.ko] undefined! ERROR: genphy_resume [drivers/net/usb/asix.ko] undefined! ERROR: phy_connect [drivers/net/usb/asix.ko] undefined! ERROR: phy_mii_ioctl [drivers/net/usb/asix.ko] undefined! ERROR: phy_ethtool_sset [drivers/net/usb/asix.ko] undefined! ERROR: mdiobus_alloc_size [drivers/net/usb/asix.ko] undefined! Caused by commit 16626b0cc3d5 (asix: Add a new driver for the AX88172A) and reverting that commit fixes the build. Sorry about that. I missed the dependency of the new driver on phylib. A fix for this problem is already in net-next, see commit 215029375c83. Thanks, Christian Thanks to Mikey for reporting this porblem. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
