| Message ID | 20240710124157.2106609-1-tmaimon77@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [linux,dev-6.6,v1] usb: chipidea: udc: enforce write to the memory. | expand |
Dear Tomer, Thank you for your patch. Am 10.07.24 um 14:41 schrieb Tomer Maimon: > In the prime endpoint function, we need to read from qh.ptr->td.token > to ensure that the previous write to it has indeed been committed > to memory. Please document the datasheet name, revision and section. If this actually caused user visible problems, please also document the test case. > Signed-off-by: Tomer Maimon <tmaimon77@gmail.com> > --- > drivers/usb/chipidea/udc.c | 15 ++++++++++++--- > 1 file changed, 12 insertions(+), 3 deletions(-) > > diff --git a/drivers/usb/chipidea/udc.c b/drivers/usb/chipidea/udc.c > index 0b7bd3c643c3..0b14a1d54d59 100644 > --- a/drivers/usb/chipidea/udc.c > +++ b/drivers/usb/chipidea/udc.c > @@ -190,12 +190,21 @@ static int hw_ep_get_halt(struct ci_hdrc *ci, int num, int dir) > * > * This function returns an error code > */ > -static int hw_ep_prime(struct ci_hdrc *ci, int num, int dir, int is_ctrl) > +static int hw_ep_prime(struct ci_hdrc *ci, struct ci_hw_ep *hwep, int num, > + int dir, int is_ctrl) > { > int n = hw_ep_bit(num, dir); > > /* Synchronize before ep prime */ > wmb(); > + > + /* > + * We add the read from qh.ptr->td.token to make sure the previous > + * write to it indeed got into the mamory so when we prime the DMA m*e*mory > + * will read the updated data > + */ > + if (hwep->qh.ptr->td.token & 0x80000000) > + pr_info("%s(): hwep->qh.ptr->td.token=%08x\n", __func__, hwep->qh.ptr->td.token); > > if (is_ctrl && dir == RX && hw_read(ci, OP_ENDPTSETUPSTAT, BIT(num))) > return -EAGAIN; > @@ -632,7 +641,7 @@ static int _hardware_enqueue(struct ci_hw_ep *hwep, struct ci_hw_req *hwreq) > hwep->qh.ptr->cap |= cpu_to_le32(mul << __ffs(QH_MULT)); > } > > - ret = hw_ep_prime(ci, hwep->num, hwep->dir, > + ret = hw_ep_prime(ci, hwep, hwep->num, hwep->dir, > hwep->type == USB_ENDPOINT_XFER_CONTROL); > done: > return ret; > @@ -658,7 +667,7 @@ static int reprime_dtd(struct ci_hdrc *ci, struct ci_hw_ep *hwep, > hwep->qh.ptr->td.token &= > cpu_to_le32(~(TD_STATUS_HALTED | TD_STATUS_ACTIVE)); > > - return hw_ep_prime(ci, hwep->num, hwep->dir, > + return hw_ep_prime(ci, hwep, hwep->num, hwep->dir, > hwep->type == USB_ENDPOINT_XFER_CONTROL); > } > Kind regards, Paul
On Wed, 2024-07-10 at 15:41 +0300, Tomer Maimon wrote: > In the prime endpoint function, we need to read from qh.ptr->td.token > to ensure that the previous write to it has indeed been committed > to memory. > > Signed-off-by: Tomer Maimon <tmaimon77@gmail.com> > --- > drivers/usb/chipidea/udc.c | 15 ++++++++++++--- > 1 file changed, 12 insertions(+), 3 deletions(-) What's the state of this patch with respect to upstream? Is this something specific to the openbmc/linux dev-6.6 tree, or is there an upstream equivalent? If the latter, can you please link to the relevant patch? Andrew
Hi Andrew, On Tue, 16 Jul 2024 at 06:34, Andrew Jeffery <andrew@codeconstruct.com.au> wrote: > > On Wed, 2024-07-10 at 15:41 +0300, Tomer Maimon wrote: > > In the prime endpoint function, we need to read from qh.ptr->td.token > > to ensure that the previous write to it has indeed been committed > > to memory. > > > > Signed-off-by: Tomer Maimon <tmaimon77@gmail.com> > > --- > > drivers/usb/chipidea/udc.c | 15 ++++++++++++--- > > 1 file changed, 12 insertions(+), 3 deletions(-) > > What's the state of this patch with respect to upstream? Is this > something specific to the openbmc/linux dev-6.6 tree, or is there an > upstream equivalent? If the latter, can you please link to the relevant > patch? This patch didn't upstream yet to OpenBMC vanilla. Its not specific to openbmc/linux dev-6.6 tree, but it is something that related to NPCM UDC module and I am not sure that Chipidea will approve to upstream it to the main chipidea driver I am planning to start the upstream in the next Linux version 6.11. > > Andrew Thanks, Tomer
Hi Paul, Thanks a lot for your comments, it will be addressed in the next version. Tomer On Wed, 10 Jul 2024 at 16:52, Paul Menzel <pmenzel@molgen.mpg.de> wrote: > > Dear Tomer, > > > Thank you for your patch. > > Am 10.07.24 um 14:41 schrieb Tomer Maimon: > > In the prime endpoint function, we need to read from qh.ptr->td.token > > to ensure that the previous write to it has indeed been committed > > to memory. > > Please document the datasheet name, revision and section. If this > actually caused user visible problems, please also document the test case. > > > Signed-off-by: Tomer Maimon <tmaimon77@gmail.com> > > --- > > drivers/usb/chipidea/udc.c | 15 ++++++++++++--- > > 1 file changed, 12 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/usb/chipidea/udc.c b/drivers/usb/chipidea/udc.c > > index 0b7bd3c643c3..0b14a1d54d59 100644 > > --- a/drivers/usb/chipidea/udc.c > > +++ b/drivers/usb/chipidea/udc.c > > @@ -190,12 +190,21 @@ static int hw_ep_get_halt(struct ci_hdrc *ci, int num, int dir) > > * > > * This function returns an error code > > */ > > -static int hw_ep_prime(struct ci_hdrc *ci, int num, int dir, int is_ctrl) > > +static int hw_ep_prime(struct ci_hdrc *ci, struct ci_hw_ep *hwep, int num, > > + int dir, int is_ctrl) > > { > > int n = hw_ep_bit(num, dir); > > > > /* Synchronize before ep prime */ > > wmb(); > > + > > + /* > > + * We add the read from qh.ptr->td.token to make sure the previous > > + * write to it indeed got into the mamory so when we prime the DMA > > m*e*mory > > > + * will read the updated data > > + */ > > + if (hwep->qh.ptr->td.token & 0x80000000) > > + pr_info("%s(): hwep->qh.ptr->td.token=%08x\n", __func__, hwep->qh.ptr->td.token); > > > > if (is_ctrl && dir == RX && hw_read(ci, OP_ENDPTSETUPSTAT, BIT(num))) > > return -EAGAIN; > > @@ -632,7 +641,7 @@ static int _hardware_enqueue(struct ci_hw_ep *hwep, struct ci_hw_req *hwreq) > > hwep->qh.ptr->cap |= cpu_to_le32(mul << __ffs(QH_MULT)); > > } > > > > - ret = hw_ep_prime(ci, hwep->num, hwep->dir, > > + ret = hw_ep_prime(ci, hwep, hwep->num, hwep->dir, > > hwep->type == USB_ENDPOINT_XFER_CONTROL); > > done: > > return ret; > > @@ -658,7 +667,7 @@ static int reprime_dtd(struct ci_hdrc *ci, struct ci_hw_ep *hwep, > > hwep->qh.ptr->td.token &= > > cpu_to_le32(~(TD_STATUS_HALTED | TD_STATUS_ACTIVE)); > > > > - return hw_ep_prime(ci, hwep->num, hwep->dir, > > + return hw_ep_prime(ci, hwep, hwep->num, hwep->dir, > > hwep->type == USB_ENDPOINT_XFER_CONTROL); > > } > > > > > Kind regards, > > Paul
On Tue, 2024-07-16 at 16:56 +0300, Tomer Maimon wrote: > Hi Andrew, > > On Tue, 16 Jul 2024 at 06:34, Andrew Jeffery > <andrew@codeconstruct.com.au> wrote: > > > > On Wed, 2024-07-10 at 15:41 +0300, Tomer Maimon wrote: > > > In the prime endpoint function, we need to read from qh.ptr->td.token > > > to ensure that the previous write to it has indeed been committed > > > to memory. > > > > > > Signed-off-by: Tomer Maimon <tmaimon77@gmail.com> > > > --- > > > drivers/usb/chipidea/udc.c | 15 ++++++++++++--- > > > 1 file changed, 12 insertions(+), 3 deletions(-) > > > > What's the state of this patch with respect to upstream? Is this > > something specific to the openbmc/linux dev-6.6 tree, or is there an > > upstream equivalent? If the latter, can you please link to the relevant > > patch? > This patch didn't upstream yet to OpenBMC vanilla. > Its not specific to openbmc/linux dev-6.6 tree, but it is something > that related to NPCM UDC module and I am not sure that Chipidea will > approve to upstream it to the main chipidea driver Given it's a driver for shared IP I prefer you get some feedback on the upstream lists before we apply this to the openbmc tree. Andrew
diff --git a/drivers/usb/chipidea/udc.c b/drivers/usb/chipidea/udc.c index 0b7bd3c643c3..0b14a1d54d59 100644 --- a/drivers/usb/chipidea/udc.c +++ b/drivers/usb/chipidea/udc.c @@ -190,12 +190,21 @@ static int hw_ep_get_halt(struct ci_hdrc *ci, int num, int dir) * * This function returns an error code */ -static int hw_ep_prime(struct ci_hdrc *ci, int num, int dir, int is_ctrl) +static int hw_ep_prime(struct ci_hdrc *ci, struct ci_hw_ep *hwep, int num, + int dir, int is_ctrl) { int n = hw_ep_bit(num, dir); /* Synchronize before ep prime */ wmb(); + + /* + * We add the read from qh.ptr->td.token to make sure the previous + * write to it indeed got into the mamory so when we prime the DMA + * will read the updated data + */ + if (hwep->qh.ptr->td.token & 0x80000000) + pr_info("%s(): hwep->qh.ptr->td.token=%08x\n", __func__, hwep->qh.ptr->td.token); if (is_ctrl && dir == RX && hw_read(ci, OP_ENDPTSETUPSTAT, BIT(num))) return -EAGAIN; @@ -632,7 +641,7 @@ static int _hardware_enqueue(struct ci_hw_ep *hwep, struct ci_hw_req *hwreq) hwep->qh.ptr->cap |= cpu_to_le32(mul << __ffs(QH_MULT)); } - ret = hw_ep_prime(ci, hwep->num, hwep->dir, + ret = hw_ep_prime(ci, hwep, hwep->num, hwep->dir, hwep->type == USB_ENDPOINT_XFER_CONTROL); done: return ret; @@ -658,7 +667,7 @@ static int reprime_dtd(struct ci_hdrc *ci, struct ci_hw_ep *hwep, hwep->qh.ptr->td.token &= cpu_to_le32(~(TD_STATUS_HALTED | TD_STATUS_ACTIVE)); - return hw_ep_prime(ci, hwep->num, hwep->dir, + return hw_ep_prime(ci, hwep, hwep->num, hwep->dir, hwep->type == USB_ENDPOINT_XFER_CONTROL); }
In the prime endpoint function, we need to read from qh.ptr->td.token to ensure that the previous write to it has indeed been committed to memory. Signed-off-by: Tomer Maimon <tmaimon77@gmail.com> --- drivers/usb/chipidea/udc.c | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-)