* Re: [PATCH 1/1] usb: gadget: Don't attempt to dequeue requests for a disabled USB endpoint on Freescale hardware
From: Simon Haggett @ 2012-10-22 11:49 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Li Yang-R58472, Greg Kroah-Hartman, linux-usb@vger.kernel.org,
linux-kernel@vger.kernel.org, balbi@ti.com,
linuxppc-dev@lists.ozlabs.org
In-Reply-To: <1535820.NU10ADjvOZ@avalon>
Hi
On 22/10/12 11:47, Laurent Pinchart wrote:
> Hi,
>
> On Monday 22 October 2012 03:33:19 Li Yang-R58472 wrote:
>> On Saturday, October 20, 2012 1:37 AM Felipe Balbi wrote:
>>> On Fri, Oct 19, 2012 at 06:19:26PM +0100, Simon Haggett wrote:
>>>> Some gadget drivers may attempt to dequeue requests for an endpoint
>>>> that has already been disabled. For example, in the UVC gadget driver,
>>>> uvc_function_set_alt() will call usb_ep_disable() when alt setting 0
>>>> is selected. When the userspace application subsequently issues the
>>>> VIDIOC_STREAMOFF ioctl, uvc_video_enable() invokes usb_ep_dequeue() to
>>>
>>> ensure that all requests have been cancelled.
>>>
>>> bug is on uvc gadget, then. Laurent ?
>
> We've discussed this topic a couple of months before. I believe that's not a
> bug.
>
> http://68.183.106.108/lists/linux-usb/msg68869.html
>
>>> Also, fsl should be removed from the tree, I'm trying to persuade iMX
>>> folks to use drivers/usb/chipidea instead.
>>
>> Besides the iMX usage, the driver is also being used by many Freescale
>> PowerPC/Coldfire SoCs. I agree that it's ideal to move to a common driver.
>> But it is a large task to make the chipidea driver works for all the
>> hardware that fsl_udc had supported and been tested on.
>>
>>>> For the Freescale High Speed Dual-Role USB controller,
>>>> fsl_ep_dequeue() provides the implementation of usb_ep_dequeue(). If
>>>> this is called for a disabled endpoint, a kernel oops will occur when
>>>
>>> the ep->ep.desc field is dereferenced (by ep_index()).
>>>
>>>> fsl_ep_disable() sets this field to NULL, as well as deleting all
>>>> pending requests for the endpoint.
>>>>
>>>> This patch adds an additional check to fsl_ep_dequeue() to ensure that
>>>> the endpoint has not already been disabled before attempting to dequeue
>>>
>>> requests.
>>>
>>>> Signed-off-by: Simon Haggett <simon.haggett@realvnc.com>
>>>> ---
>>>>
>>>> drivers/usb/gadget/fsl_udc_core.c | 5 ++++-
>>>> 1 files changed, 4 insertions(+), 1 deletions(-)
>>>>
>>>> diff --git a/drivers/usb/gadget/fsl_udc_core.c
>>>> b/drivers/usb/gadget/fsl_udc_core.c
>>>> index 6ae70cb..acd513b 100644
>>>> --- a/drivers/usb/gadget/fsl_udc_core.c
>>>> +++ b/drivers/usb/gadget/fsl_udc_core.c
>>>> @@ -955,7 +955,10 @@ static int fsl_ep_dequeue(struct usb_ep *_ep,
>>> struct usb_request *_req)
>>>
>>>> int ep_num, stopped, ret = 0;
>>>> u32 epctrl;
>>>>
>>>> - if (!_ep || !_req)
>>>> + /* Ensure that the ep and request are valid, and the ep is not
>>>> + * disabled
>>>> + */
>>>> + if (!_ep || !_req || !ep->ep.desc)
>>>> return -EINVAL;
>
> Shouldn't that last check be done with a lock taken ?
I had presumed a lock wasn't necessary because ep->ep.desc is only set
to NULL by fsl_ep_disable() which, since it is called by
usb_ep_disable(), should only be invoked when no other task is using the
endpoint (according to include/linux/usb/gadget.h). Furthermore, the
chipidea UDC driver does check the equivalent of this field is not NULL
without taking a lock (ep_dequeue() in drivers/usb/chipidea/udc.c).
However, it is possible that I'm misunderstanding something here, so
apologies if I am.
>
>>>> spin_lock_irqsave(&ep->udc->lock, flags);
>
Many thanks
Simon
^ permalink raw reply
* Re: [PATCH 1/1] usb: gadget: Don't attempt to dequeue requests for a disabled USB endpoint on Freescale hardware
From: Felipe Balbi @ 2012-10-22 10:56 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Simon Haggett, Li Yang-R58472, Greg Kroah-Hartman,
linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org,
balbi@ti.com, linuxppc-dev@lists.ozlabs.org
In-Reply-To: <1535820.NU10ADjvOZ@avalon>
[-- Attachment #1: Type: text/plain, Size: 1189 bytes --]
Hi,
On Mon, Oct 22, 2012 at 12:47:21PM +0200, Laurent Pinchart wrote:
> Hi,
>
> On Monday 22 October 2012 03:33:19 Li Yang-R58472 wrote:
> > On Saturday, October 20, 2012 1:37 AM Felipe Balbi wrote:
> > > On Fri, Oct 19, 2012 at 06:19:26PM +0100, Simon Haggett wrote:
> > > > Some gadget drivers may attempt to dequeue requests for an endpoint
> > > > that has already been disabled. For example, in the UVC gadget driver,
> > > > uvc_function_set_alt() will call usb_ep_disable() when alt setting 0
> > > > is selected. When the userspace application subsequently issues the
> > > > VIDIOC_STREAMOFF ioctl, uvc_video_enable() invokes usb_ep_dequeue() to
> > >
> > > ensure that all requests have been cancelled.
> > >
> > > bug is on uvc gadget, then. Laurent ?
>
> We've discussed this topic a couple of months before. I believe that's not a
> bug.
>
> http://68.183.106.108/lists/linux-usb/msg68869.html
fair enough :-)
That's a different case, however. At the link above we're discussing
dequeueing a request which is already being dequeued. $SUBJECT is trying
to fix dequeueing of a request for an endpoint which isn't even enabled.
--
balbi
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH 1/1] usb: gadget: Don't attempt to dequeue requests for a disabled USB endpoint on Freescale hardware
From: Laurent Pinchart @ 2012-10-22 10:47 UTC (permalink / raw)
To: Li Yang-R58472
Cc: Simon Haggett, Greg Kroah-Hartman, linux-usb@vger.kernel.org,
linux-kernel@vger.kernel.org, balbi@ti.com,
linuxppc-dev@lists.ozlabs.org
In-Reply-To: <94F013E7935FF44C83EBE7784D62AD3F094730DB@039-SN2MPN1-023.039d.mgd.msft.net>
Hi,
On Monday 22 October 2012 03:33:19 Li Yang-R58472 wrote:
> On Saturday, October 20, 2012 1:37 AM Felipe Balbi wrote:
> > On Fri, Oct 19, 2012 at 06:19:26PM +0100, Simon Haggett wrote:
> > > Some gadget drivers may attempt to dequeue requests for an endpoint
> > > that has already been disabled. For example, in the UVC gadget driver,
> > > uvc_function_set_alt() will call usb_ep_disable() when alt setting 0
> > > is selected. When the userspace application subsequently issues the
> > > VIDIOC_STREAMOFF ioctl, uvc_video_enable() invokes usb_ep_dequeue() to
> >
> > ensure that all requests have been cancelled.
> >
> > bug is on uvc gadget, then. Laurent ?
We've discussed this topic a couple of months before. I believe that's not a
bug.
http://68.183.106.108/lists/linux-usb/msg68869.html
> > Also, fsl should be removed from the tree, I'm trying to persuade iMX
> > folks to use drivers/usb/chipidea instead.
>
> Besides the iMX usage, the driver is also being used by many Freescale
> PowerPC/Coldfire SoCs. I agree that it's ideal to move to a common driver.
> But it is a large task to make the chipidea driver works for all the
> hardware that fsl_udc had supported and been tested on.
>
> > > For the Freescale High Speed Dual-Role USB controller,
> > > fsl_ep_dequeue() provides the implementation of usb_ep_dequeue(). If
> > > this is called for a disabled endpoint, a kernel oops will occur when
> >
> > the ep->ep.desc field is dereferenced (by ep_index()).
> >
> > > fsl_ep_disable() sets this field to NULL, as well as deleting all
> > > pending requests for the endpoint.
> > >
> > > This patch adds an additional check to fsl_ep_dequeue() to ensure that
> > > the endpoint has not already been disabled before attempting to dequeue
> >
> > requests.
> >
> > > Signed-off-by: Simon Haggett <simon.haggett@realvnc.com>
> > > ---
> > >
> > > drivers/usb/gadget/fsl_udc_core.c | 5 ++++-
> > > 1 files changed, 4 insertions(+), 1 deletions(-)
> > >
> > > diff --git a/drivers/usb/gadget/fsl_udc_core.c
> > > b/drivers/usb/gadget/fsl_udc_core.c
> > > index 6ae70cb..acd513b 100644
> > > --- a/drivers/usb/gadget/fsl_udc_core.c
> > > +++ b/drivers/usb/gadget/fsl_udc_core.c
> > > @@ -955,7 +955,10 @@ static int fsl_ep_dequeue(struct usb_ep *_ep,
> > struct usb_request *_req)
> >
> > > int ep_num, stopped, ret = 0;
> > > u32 epctrl;
> > >
> > > - if (!_ep || !_req)
> > > + /* Ensure that the ep and request are valid, and the ep is not
> > > + * disabled
> > > + */
> > > + if (!_ep || !_req || !ep->ep.desc)
> > > return -EINVAL;
Shouldn't that last check be done with a lock taken ?
> > > spin_lock_irqsave(&ep->udc->lock, flags);
--
Regards,
Laurent Pinchart
^ permalink raw reply
* Re: [PATCH 5/7] usb: gadget: fsl_udc: postpone freeing current dTD
From: Felipe Balbi @ 2012-10-22 7:54 UTC (permalink / raw)
To: Christoph Fritz
Cc: Estevam Fabio-R49496, Li Yang-R58472, Greg Kroah-Hartman,
Chen Peter-B29397, Sascha Hauer, linux-usb, balbi, Hans J. Koch,
Daniel Mack, Christian Hemp, linuxppc-dev, Teresa Gamez,
Sebastian Andrzej Siewior
In-Reply-To: <1350760347.3669.3.camel@mars>
[-- Attachment #1: Type: text/plain, Size: 559 bytes --]
Hi,
On Sat, Oct 20, 2012 at 09:12:27PM +0200, Christoph Fritz wrote:
> On Fri, 2012-10-19 at 13:44 +0300, Felipe Balbi wrote:
> > > I thought about this too but wasn't able to use chipidea with
> > > MXC_EHCI_INTERNAL_PHY as it's called in fsl_udc.
> >
> > that's a matter of writing the PHY driver, right ;-) It has nothing to
> > do with chipidea, actually :-)
>
> Okay, I'll do. But then we should purge the old buggy fsl_udc.
I'm all for that. We shouldn't have multiple copies of a single driver
hanging around.
cheers
--
balbi
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH 1/1] usb: gadget: Don't attempt to dequeue requests for a disabled USB endpoint on Freescale hardware
From: Felipe Balbi @ 2012-10-22 7:53 UTC (permalink / raw)
To: Li Yang-R58472
Cc: Simon Haggett, Greg Kroah-Hartman, linux-usb@vger.kernel.org,
linux-kernel@vger.kernel.org, balbi@ti.com, Laurent Pinchart,
linuxppc-dev@lists.ozlabs.org
In-Reply-To: <94F013E7935FF44C83EBE7784D62AD3F094730DB@039-SN2MPN1-023.039d.mgd.msft.net>
[-- Attachment #1: Type: text/plain, Size: 1964 bytes --]
On Mon, Oct 22, 2012 at 03:33:19AM +0000, Li Yang-R58472 wrote:
>
>
> > -----Original Message-----
> > From: Felipe Balbi [mailto:balbi@ti.com]
> > Sent: Saturday, October 20, 2012 1:37 AM
> > To: Simon Haggett
> > Cc: Li Yang-R58472; Felipe Balbi; Greg Kroah-Hartman; linux-
> > usb@vger.kernel.org; linuxppc-dev@lists.ozlabs.org; linux-
> > kernel@vger.kernel.org; Laurent Pinchart
> > Subject: Re: [PATCH 1/1] usb: gadget: Don't attempt to dequeue requests
> > for a disabled USB endpoint on Freescale hardware
> >
> > Hi,
> >
> > On Fri, Oct 19, 2012 at 06:19:26PM +0100, Simon Haggett wrote:
> > > Some gadget drivers may attempt to dequeue requests for an endpoint
> > > that has already been disabled. For example, in the UVC gadget driver,
> > > uvc_function_set_alt() will call usb_ep_disable() when alt setting 0
> > > is selected. When the userspace application subsequently issues the
> > > VIDIOC_STREAMOFF ioctl, uvc_video_enable() invokes usb_ep_dequeue() to
> > ensure that all requests have been cancelled.
> >
> > bug is on uvc gadget, then. Laurent ?
> >
> > Also, fsl should be removed from the tree, I'm trying to persuade iMX
> > folks to use drivers/usb/chipidea instead.
>
> Besides the iMX usage, the driver is also being used by many Freescale
> PowerPC/Coldfire SoCs. I agree that it's ideal to move to a common
> driver. But it is a large task to make the chipidea driver works for
> all the hardware that fsl_udc had supported and been tested on.
I understand that, but we just can't keep so many duplicated drivers in
mainline. chipidea udc had at least 3 different implementations. Now
it's the time to combine all of those and stick to a single driver.
Just make a plan to slowly move towards chipidea in the upcoming few
merge windows. I can continue to take in bugfixes for fsl_udc, but only
if I see that you guys are working towards merging with chipidea driver.
--
balbi
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH 8/10] memory-hotplug : remove page table of x86_64 architecture
From: Wen Congyang @ 2012-10-22 7:11 UTC (permalink / raw)
To: wujianguo
Cc: linux-s390, linux-ia64, jiang.liu, len.brown, linux-acpi,
linux-sh, wujianguo, x86, linux-kernel, cmetcalf, linux-mm,
Yasuaki Ishimatsu, minchan.kim, kosaki.motohiro, rientjes,
sparclinux, qiuxishi, cl, linuxppc-dev, akpm, liuj97
In-Reply-To: <5073DFC0.3010400@gmail.com>
Hi, Wu
Sorry for late reply.
At 10/09/2012 04:26 PM, wujianguo Wrote:
> Hi Congyang,
> I think we should also free pages which are used by page tables after removing
> page tables of the memory.
It is OK to do it.
>
> From: Jianguo Wu <wujianguo@huawei.com>
>
> Signed-off-by: Jianguo Wu <wujianguo@huawei.com>
> Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
> ---
> arch/x86/mm/init_64.c | 110 +++++++++++++++++++++++++++++++++++++++---------
> 1 files changed, 89 insertions(+), 21 deletions(-)
>
> diff --git a/arch/x86/mm/init_64.c b/arch/x86/mm/init_64.c
> index 5596dfa..81f9c3b 100644
> --- a/arch/x86/mm/init_64.c
> +++ b/arch/x86/mm/init_64.c
> @@ -675,6 +675,74 @@ int arch_add_memory(int nid, u64 start, u64 size)
> }
> EXPORT_SYMBOL_GPL(arch_add_memory);
>
> +static inline void free_pagetable(struct page *page)
> +{
> + struct zone *zone;
> +
> + __ClearPageReserved(page);
> + __free_page(page);
> +
> + zone = page_zone(page);
> + zone_span_writelock(zone);
> + zone->present_pages++;
> + zone_span_writeunlock(zone);
> + totalram_pages++;
Why do you update zone and totalram_pages here?
> +}
> +
> +static void free_pte_table(pte_t *pte_start, pmd_t *pmd)
> +{
> + pte_t *pte;
> + int i;
> +
> + for (i = 0; i < PTRS_PER_PTE; i++) {
> + pte = pte_start + i;
> + if (pte_val(*pte))
> + break;
> + }
> +
> + /* free a pte talbe */
> + if (i == PTRS_PER_PTE) {
> + free_pagetable(pmd_page(*pmd));
The memory may be allocated at booting. So it is very dangerous to
free it without any check.
> + pmd_clear(pmd);
> + }
> +}
> +
> +static void free_pmd_table(pmd_t *pmd_start, pud_t *pud)
> +{
> + pmd_t *pmd;
> + int i;
> +
> + for (i = 0; i < PTRS_PER_PMD; i++) {
> + pmd = pmd_start + i;
> + if (pmd_val(*pmd))
> + break;
> + }
> +
> + /* free a pmd talbe */
> + if (i == PTRS_PER_PMD) {
> + free_pagetable(pud_page(*pud));
> + pud_clear(pud);
> + }
> +}
> +
> +static void free_pud_table(pud_t *pud_start, pgd_t *pgd)
> +{
> + pud_t *pud;
> + int i;
> +
> + for (i = 0; i < PTRS_PER_PUD; i++) {
> + pud = pud_start + i;
> + if (pud_val(*pud))
> + break;
> + }
> +
> + /* free a pud table */
> + if (i == PTRS_PER_PUD) {
> + free_pagetable(pgd_page(*pgd));
> + pgd_clear(pgd);
> + }
> +}
> +
> static void __meminit
> phys_pte_remove(pte_t *pte_page, unsigned long addr, unsigned long end)
> {
> @@ -704,21 +772,19 @@ phys_pmd_remove(pmd_t *pmd_page, unsigned long addr, unsigned long end)
> unsigned long pages = 0, next;
> int i = pmd_index(addr);
>
> - for (; i < PTRS_PER_PMD; i++, addr = next) {
> + for (; i < PTRS_PER_PMD && addr < end; i++, addr = next) {
> unsigned long pte_phys;
> pmd_t *pmd = pmd_page + pmd_index(addr);
> pte_t *pte;
>
> - if (addr >= end)
> - break;
> -
> - next = (addr & PMD_MASK) + PMD_SIZE;
> + next = pmd_addr_end(addr, end);
>
> if (!pmd_present(*pmd))
> continue;
>
> if (pmd_large(*pmd)) {
> - if ((addr & ~PMD_MASK) == 0 && next <= end) {
> + if (IS_ALIGNED(addr, PMD_SIZE) &&
> + IS_ALIGNED(next, PMD_SIZE)) {
> set_pmd(pmd, __pmd(0));
> pages++;
> continue;
> @@ -729,7 +795,8 @@ phys_pmd_remove(pmd_t *pmd_page, unsigned long addr, unsigned long end)
> * so split 2M page to 4K page.
> */
> pte = alloc_low_page(&pte_phys);
> - __split_large_page((pte_t *)pmd, addr, pte);
> + __split_large_page((pte_t *)pmd,
> + (unsigned long)__va(addr), pte);
>
> spin_lock(&init_mm.page_table_lock);
> pmd_populate_kernel(&init_mm, pmd, __va(pte_phys));
> @@ -738,7 +805,8 @@ phys_pmd_remove(pmd_t *pmd_page, unsigned long addr, unsigned long end)
>
> spin_lock(&init_mm.page_table_lock);
> pte = map_low_page((pte_t *)pmd_page_vaddr(*pmd));
> - phys_pte_remove(pte, addr, end);
> + phys_pte_remove(pte, addr, next);
> + free_pte_table(pte, pmd);
> unmap_low_page(pte);
> spin_unlock(&init_mm.page_table_lock);
> }
> @@ -751,21 +819,19 @@ phys_pud_remove(pud_t *pud_page, unsigned long addr, unsigned long end)
> unsigned long pages = 0, next;
> int i = pud_index(addr);
>
> - for (; i < PTRS_PER_PUD; i++, addr = next) {
> + for (; i < PTRS_PER_PUD && addr < end; i++, addr = next) {
> unsigned long pmd_phys;
> pud_t *pud = pud_page + pud_index(addr);
> pmd_t *pmd;
>
> - if (addr >= end)
> - break;
> -
> - next = (addr & PUD_MASK) + PUD_SIZE;
> + next = pud_addr_end(addr, end);
>
> if (!pud_present(*pud))
> continue;
>
> if (pud_large(*pud)) {
> - if ((addr & ~PUD_MASK) == 0 && next <= end) {
> + if (IS_ALIGNED(addr, PUD_SIZE) &&
> + IS_ALIGNED(next, PUD_SIZE)) {
> set_pud(pud, __pud(0));
> pages++;
> continue;
> @@ -776,15 +842,18 @@ phys_pud_remove(pud_t *pud_page, unsigned long addr, unsigned long end)
> * so split 1G page to 2M page.
> */
> pmd = alloc_low_page(&pmd_phys);
> - __split_large_page((pte_t *)pud, addr, (pte_t *)pmd);
> + __split_large_page((pte_t *)pud,
> + (unsigned long)__va(addr),
> + (pte_t *)pmd);
>
> spin_lock(&init_mm.page_table_lock);
> pud_populate(&init_mm, pud, __va(pmd_phys));
> spin_unlock(&init_mm.page_table_lock);
> }
>
> - pmd = map_low_page(pmd_offset(pud, 0));
> - phys_pmd_remove(pmd, addr, end);
> + pmd = map_low_page((pmd_t *)pud_page_vaddr(*pud));
Hmm, pmd_offset(pud, 0) is equal to (pmd_t *)pud_page_vaddr(*pud).
Is it OK to merge your patch into my patch?
Thanks
Wen Congyang
> + phys_pmd_remove(pmd, addr, next);
> + free_pmd_table(pmd, pud);
> unmap_low_page(pmd);
> __flush_tlb_all();
> }
> @@ -805,15 +874,14 @@ kernel_physical_mapping_remove(unsigned long start, unsigned long end)
> pgd_t *pgd = pgd_offset_k(start);
> pud_t *pud;
>
> - next = (start + PGDIR_SIZE) & PGDIR_MASK;
> - if (next > end)
> - next = end;
> + next = pgd_addr_end(start, end);
>
> if (!pgd_present(*pgd))
> continue;
>
> pud = map_low_page((pud_t *)pgd_page_vaddr(*pgd));
> - phys_pud_remove(pud, __pa(start), __pa(end));
> + phys_pud_remove(pud, __pa(start), __pa(next));
> + free_pud_table(pud, pgd);
> unmap_low_page(pud);
> }
>
> -- 1.7.6.1 .
>
>
> On 2012-10-5 10:36, Yasuaki Ishimatsu wrote:
>> From: Wen Congyang <wency@cn.fujitsu.com>
>>
>> For hot removing memory, we sholud remove page table about the memory.
>> So the patch searches a page table about the removed memory, and clear
>> page table.
>>
>> CC: David Rientjes <rientjes@google.com>
>> CC: Jiang Liu <liuj97@gmail.com>
>> CC: Len Brown <len.brown@intel.com>
>> CC: Christoph Lameter <cl@linux.com>
>> Cc: Minchan Kim <minchan.kim@gmail.com>
>> CC: Andrew Morton <akpm@linux-foundation.org>
>> CC: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
>> CC: Yasuaki Ishimatsu <isimatu.yasuaki@jp.fujitsu.com>
>> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
>> ---
>> arch/x86/include/asm/pgtable_types.h | 1
>> arch/x86/mm/init_64.c | 147 +++++++++++++++++++++++++++++++++++
>> arch/x86/mm/pageattr.c | 47 +++++------
>> 3 files changed, 173 insertions(+), 22 deletions(-)
>>
>> Index: linux-3.6/arch/x86/mm/init_64.c
>> ===================================================================
>> --- linux-3.6.orig/arch/x86/mm/init_64.c 2012-10-04 18:30:21.171698416 +0900
>> +++ linux-3.6/arch/x86/mm/init_64.c 2012-10-04 18:30:27.317704652 +0900
>> @@ -675,6 +675,151 @@ int arch_add_memory(int nid, u64 start,
>> }
>> EXPORT_SYMBOL_GPL(arch_add_memory);
>>
>> +static void __meminit
>> +phys_pte_remove(pte_t *pte_page, unsigned long addr, unsigned long end)
>> +{
>> + unsigned pages = 0;
>> + int i = pte_index(addr);
>> +
>> + pte_t *pte = pte_page + pte_index(addr);
>> +
>> + for (; i < PTRS_PER_PTE; i++, addr += PAGE_SIZE, pte++) {
>> +
>> + if (addr >= end)
>> + break;
>> +
>> + if (!pte_present(*pte))
>> + continue;
>> +
>> + pages++;
>> + set_pte(pte, __pte(0));
>> + }
>> +
>> + update_page_count(PG_LEVEL_4K, -pages);
>> +}
>> +
>> +static void __meminit
>> +phys_pmd_remove(pmd_t *pmd_page, unsigned long addr, unsigned long end)
>> +{
>> + unsigned long pages = 0, next;
>> + int i = pmd_index(addr);
>> +
>> + for (; i < PTRS_PER_PMD; i++, addr = next) {
>> + unsigned long pte_phys;
>> + pmd_t *pmd = pmd_page + pmd_index(addr);
>> + pte_t *pte;
>> +
>> + if (addr >= end)
>> + break;
>> +
>> + next = (addr & PMD_MASK) + PMD_SIZE;
>> +
>> + if (!pmd_present(*pmd))
>> + continue;
>> +
>> + if (pmd_large(*pmd)) {
>> + if ((addr & ~PMD_MASK) == 0 && next <= end) {
>> + set_pmd(pmd, __pmd(0));
>> + pages++;
>> + continue;
>> + }
>> +
>> + /*
>> + * We use 2M page, but we need to remove part of them,
>> + * so split 2M page to 4K page.
>> + */
>> + pte = alloc_low_page(&pte_phys);
>> + __split_large_page((pte_t *)pmd, addr, pte);
>> +
>> + spin_lock(&init_mm.page_table_lock);
>> + pmd_populate_kernel(&init_mm, pmd, __va(pte_phys));
>> + spin_unlock(&init_mm.page_table_lock);
>> + }
>> +
>> + spin_lock(&init_mm.page_table_lock);
>> + pte = map_low_page((pte_t *)pmd_page_vaddr(*pmd));
>> + phys_pte_remove(pte, addr, end);
>> + unmap_low_page(pte);
>> + spin_unlock(&init_mm.page_table_lock);
>> + }
>> + update_page_count(PG_LEVEL_2M, -pages);
>> +}
>> +
>> +static void __meminit
>> +phys_pud_remove(pud_t *pud_page, unsigned long addr, unsigned long end)
>> +{
>> + unsigned long pages = 0, next;
>> + int i = pud_index(addr);
>> +
>> + for (; i < PTRS_PER_PUD; i++, addr = next) {
>> + unsigned long pmd_phys;
>> + pud_t *pud = pud_page + pud_index(addr);
>> + pmd_t *pmd;
>> +
>> + if (addr >= end)
>> + break;
>> +
>> + next = (addr & PUD_MASK) + PUD_SIZE;
>> +
>> + if (!pud_present(*pud))
>> + continue;
>> +
>> + if (pud_large(*pud)) {
>> + if ((addr & ~PUD_MASK) == 0 && next <= end) {
>> + set_pud(pud, __pud(0));
>> + pages++;
>> + continue;
>> + }
>> +
>> + /*
>> + * We use 1G page, but we need to remove part of them,
>> + * so split 1G page to 2M page.
>> + */
>> + pmd = alloc_low_page(&pmd_phys);
>> + __split_large_page((pte_t *)pud, addr, (pte_t *)pmd);
>> +
>> + spin_lock(&init_mm.page_table_lock);
>> + pud_populate(&init_mm, pud, __va(pmd_phys));
>> + spin_unlock(&init_mm.page_table_lock);
>> + }
>> +
>> + pmd = map_low_page(pmd_offset(pud, 0));
>> + phys_pmd_remove(pmd, addr, end);
>> + unmap_low_page(pmd);
>> + __flush_tlb_all();
>> + }
>> + __flush_tlb_all();
>> +
>> + update_page_count(PG_LEVEL_1G, -pages);
>> +}
>> +
>> +void __meminit
>> +kernel_physical_mapping_remove(unsigned long start, unsigned long end)
>> +{
>> + unsigned long next;
>> +
>> + start = (unsigned long)__va(start);
>> + end = (unsigned long)__va(end);
>> +
>> + for (; start < end; start = next) {
>> + pgd_t *pgd = pgd_offset_k(start);
>> + pud_t *pud;
>> +
>> + next = (start + PGDIR_SIZE) & PGDIR_MASK;
>> + if (next > end)
>> + next = end;
>> +
>> + if (!pgd_present(*pgd))
>> + continue;
>> +
>> + pud = map_low_page((pud_t *)pgd_page_vaddr(*pgd));
>> + phys_pud_remove(pud, __pa(start), __pa(end));
>> + unmap_low_page(pud);
>> + }
>> +
>> + __flush_tlb_all();
>> +}
>> +
>> #ifdef CONFIG_MEMORY_HOTREMOVE
>> int __ref arch_remove_memory(u64 start, u64 size)
>> {
>> @@ -687,6 +832,8 @@ int __ref arch_remove_memory(u64 start,
>> ret = __remove_pages(zone, start_pfn, nr_pages);
>> WARN_ON_ONCE(ret);
>>
>> + kernel_physical_mapping_remove(start, start + size);
>> +
>> return ret;
>> }
>> #endif
>> Index: linux-3.6/arch/x86/include/asm/pgtable_types.h
>> ===================================================================
>> --- linux-3.6.orig/arch/x86/include/asm/pgtable_types.h 2012-10-04 18:26:51.925486954 +0900
>> +++ linux-3.6/arch/x86/include/asm/pgtable_types.h 2012-10-04 18:30:27.322704656 +0900
>> @@ -334,6 +334,7 @@ static inline void update_page_count(int
>> * as a pte too.
>> */
>> extern pte_t *lookup_address(unsigned long address, unsigned int *level);
>> +extern int __split_large_page(pte_t *kpte, unsigned long address, pte_t *pbase);
>>
>> #endif /* !__ASSEMBLY__ */
>>
>> Index: linux-3.6/arch/x86/mm/pageattr.c
>> ===================================================================
>> --- linux-3.6.orig/arch/x86/mm/pageattr.c 2012-10-04 18:26:51.923486952 +0900
>> +++ linux-3.6/arch/x86/mm/pageattr.c 2012-10-04 18:30:27.328704662 +0900
>> @@ -501,21 +501,13 @@ out_unlock:
>> return do_split;
>> }
>>
>> -static int split_large_page(pte_t *kpte, unsigned long address)
>> +int __split_large_page(pte_t *kpte, unsigned long address, pte_t *pbase)
>> {
>> unsigned long pfn, pfninc = 1;
>> unsigned int i, level;
>> - pte_t *pbase, *tmp;
>> + pte_t *tmp;
>> pgprot_t ref_prot;
>> - struct page *base;
>> -
>> - if (!debug_pagealloc)
>> - spin_unlock(&cpa_lock);
>> - base = alloc_pages(GFP_KERNEL | __GFP_NOTRACK, 0);
>> - if (!debug_pagealloc)
>> - spin_lock(&cpa_lock);
>> - if (!base)
>> - return -ENOMEM;
>> + struct page *base = virt_to_page(pbase);
>>
>> spin_lock(&pgd_lock);
>> /*
>> @@ -523,10 +515,11 @@ static int split_large_page(pte_t *kpte,
>> * up for us already:
>> */
>> tmp = lookup_address(address, &level);
>> - if (tmp != kpte)
>> - goto out_unlock;
>> + if (tmp != kpte) {
>> + spin_unlock(&pgd_lock);
>> + return 1;
>> + }
>>
>> - pbase = (pte_t *)page_address(base);
>> paravirt_alloc_pte(&init_mm, page_to_pfn(base));
>> ref_prot = pte_pgprot(pte_clrhuge(*kpte));
>> /*
>> @@ -579,17 +572,27 @@ static int split_large_page(pte_t *kpte,
>> * going on.
>> */
>> __flush_tlb_all();
>> + spin_unlock(&pgd_lock);
>>
>> - base = NULL;
>> + return 0;
>> +}
>>
>> -out_unlock:
>> - /*
>> - * If we dropped out via the lookup_address check under
>> - * pgd_lock then stick the page back into the pool:
>> - */
>> - if (base)
>> +static int split_large_page(pte_t *kpte, unsigned long address)
>> +{
>> + pte_t *pbase;
>> + struct page *base;
>> +
>> + if (!debug_pagealloc)
>> + spin_unlock(&cpa_lock);
>> + base = alloc_pages(GFP_KERNEL | __GFP_NOTRACK, 0);
>> + if (!debug_pagealloc)
>> + spin_lock(&cpa_lock);
>> + if (!base)
>> + return -ENOMEM;
>> +
>> + pbase = (pte_t *)page_address(base);
>> + if (__split_large_page(kpte, address, pbase))
>> __free_page(base);
>> - spin_unlock(&pgd_lock);
>>
>> return 0;
>> }
>>
>> --
>> To unsubscribe, send a message with 'unsubscribe linux-mm' in
>> the body to majordomo@kvack.org. For more info on Linux MM,
>> see: http://www.linux-mm.org/ .
>> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
>>
>
>
^ permalink raw reply
* Re: [RFC PATCH powerpc] Fix a lazy irq related WARING in arch_local_irq_restore()
From: Li Zhong @ 2012-10-22 7:23 UTC (permalink / raw)
To: Benjamin Herrenschmidt
Cc: Paul Mackerras, Paul E. McKenney, PowerPC email list
In-Reply-To: <1350518056.4678.113.camel@pasglop>
On Thu, 2012-10-18 at 10:54 +1100, Benjamin Herrenschmidt wrote:
> On Wed, 2012-09-26 at 18:10 +0800, Li Zhong wrote:
>
> ../...
>
> Sorry got distracted, got back on this patch today:
>
> > > We might need to "sanitize" the enable state in the PACA before we
> > > actually enter NAP or in the return from NAP code, like we do for normal
> > > idle code...
> >
> > Hi Ben,
> >
> > After some further reading of the code, I updated the code as following,
> > but I'm not very sure, and guess it most probably has some issues ...
> > Could you please help to review and give your comments?
>
> I think it's still not right, see below
>
> > In extended_cede_processor(), if there is still lazy irq pending, I used
> > local_irq_enable() to make sure the irq replayed and flags cleared, but
> > I'm not sure whether it is a proper way.
>
> Right but that will break things for idle. In idle, if you have
> re-enabled interrupts, you need to return and essentially abort the
> attempt at going to nap mode, because the interrupt might have set need
> resched. That's why we normally just check if something's pending and
> return, letting the upper levels re-enable interrupts and do all the
> dirty work for us.
>
> Now, hotplug might differ here in what it needs, but in any case,
> extended_cede_processor doesn't seem to be the right place to handle it,
> at best that function should return if it thinks there's something that
> needs to be done and let the upper layers deal with it appropriately.
>
> > In pseries_mach_cpu_die(), I added local_irq_disable() after cede, and
> > prepare for the start_secondary_resume(), but I'm not sure whether we
> > also need a hard_irq_disable() here.
>
> You probably do if it's going to go back to the start secondary path. It
> shouldn't hurt in any case as long as start_secondary_resume()
> eventually does a local_irq_enable().
>
> > I'm still a little confused by the meaning of PACA_IRQ_HARD_DIS in
> > irq_happened. From the checking at the warning point, it seems only
> > irq_happened equaling 0x1(PACA_IRQ_HARD_DIS) means hard irqs are
> > disabled.
Hi Ben,
Below are my current understandings and a few more questions, please
correct me if there are any misunderstandings. Thank you.
> No. They are disabled if any bit in there corresponding to a "level"
> interrupt is set as well. Only the "edge" interrupts (and decrementer
> which we treat as edge and reset to a high value when it kicks) are
> ignored for the sake of HW irq state.
>From the code, it seems that the hardware_interrupt/external_input
(corresponding to PACA_IRQ_EE) are "level" interrupts. For "level"
interrupts, is it because we will see it again, so we need to hard
disable? or else we might enter into an infinite loop?
> The reason we have this IRQ_HARD_DIS bit is to indicate that a 'manual'
> hard disabling occurred (by opposition to one happening as a result of
> an external interrupt).
The external interrupt causing the hard disabling, is done in the code
of masked_##_H##interrupt: for book3s, or masked_interrupt_book3e
PACA_IRQ_EE 1 for book3e ?
> We need that so we can avoid doing an mfspr() in local_irq_enable() and
> entirely rely on the content of irq_happened to know whether interrupts
> are hard enabled or hard disabled.
Is this about the code in arch_local_irq_restore()? so if (!
irq_happended), we could return directly as we know it is hard enabled.
And here
if (unlikely(irq_happened != PACA_IRQ_HARD_DIS))
__hard_irq_disable();
We don't need to hard disable if (irq_happened == PACA_IRQ_HARD_DIS), as
we know it is hard disabled ('manual').
Then here, can we save a few more mtmsrd by also checking PACA_IRQ_EE
bit? like following:
- if (unlikely(irq_happened != PACA_IRQ_HARD_DIS))
+ if (unlikely(!(irq_happened & (PACA_IRQ_HARD_DIS | PACA_IRQ_EE))))
> We do that because mfspr is a fairly expensive instruction. But that
> means that we need to make sure we always have a consistent content in
> irq_happened. That's also why I've added all those sanity checks if you
> enable IRQ tracing.
See.
> > Is it possible to set this bit at anyplace the hard irqs are disabled,
> > so then we could check whether this bit is set to know whether hard irqs
> > are disabled? Then it seems that in MASKED_INTERRUPT, we need set this
> > bit where MSR_EE is cleared for something other than decrementer. Maybe
> > I missed too much things?
>
> Either that bit or PACA_IRQ_EE. Both indicate that interrupts are hard
> disabled.
>
> There are some rare cases where do do change MSR:EE without touching
> those bits, only when we're going to restore it shortly afterward in the
> kernel asm exception entry/exit path for example.
I don't get it very clearly here. I might need some more time to read
and understand all the related asm codes.
Currently, it seems to me in EXCEPTION_COMMON, SOFT_DISABLE_INTS is
called to set PACA_IRQ_HARD_DIS, and other bits might be set when
__SOFTEN_TEST (or masked_interrupt_book3e) is called. And in the
exception exit path, something like
SOFT_DISABLE_INTS, .restore_interrupts, restore_check_irq_replay, etc
are called to handle the irq_happened bits.
Thanks, Zhong
> Cheers,
> Ben.
>
> > Thanks, Zhong
> >
> > ===================
> > diff --git a/arch/powerpc/platforms/pseries/hotplug-cpu.c b/arch/powerpc/platforms/pseries/hotplug-cpu.c
> > index 64c97d8..b5f7597 100644
> > --- a/arch/powerpc/platforms/pseries/hotplug-cpu.c
> > +++ b/arch/powerpc/platforms/pseries/hotplug-cpu.c
> > @@ -130,6 +130,8 @@ static void pseries_mach_cpu_die(void)
> > extended_cede_processor(cede_latency_hint);
> > }
> >
> > + local_irq_disable();
> > +
> > if (!get_lppaca()->shared_proc)
> > get_lppaca()->donate_dedicated_cpu = 0;
> > get_lppaca()->idle = 0;
> > diff --git a/arch/powerpc/platforms/pseries/plpar_wrappers.h b/arch/powerpc/platforms/pseries/plpar_wrappers.h
> > index 13e8cc4..07560d8 100644
> > --- a/arch/powerpc/platforms/pseries/plpar_wrappers.h
> > +++ b/arch/powerpc/platforms/pseries/plpar_wrappers.h
> > @@ -2,6 +2,7 @@
> > #define _PSERIES_PLPAR_WRAPPERS_H
> >
> > #include <linux/string.h>
> > +#include <linux/irqflags.h>
> >
> > #include <asm/hvcall.h>
> > #include <asm/paca.h>
> > @@ -41,7 +42,19 @@ static inline long extended_cede_processor(unsigned long latency_hint)
> > u8 old_latency_hint = get_cede_latency_hint();
> >
> > set_cede_latency_hint(latency_hint);
> > +
> > + while (!prep_irq_for_idle()) {
> > + local_irq_enable();
> > + local_irq_disable();
> > + }
> > +
> > rc = cede_processor();
> > +#ifdef CONFIG_TRACE_IRQFLAGS
> > + /* Ensure that H_CEDE returns with IRQs on */
> > + if (WARN_ON(!(mfmsr() & MSR_EE)))
> > + __hard_irq_enable();
> > +#endif
> > +
> > set_cede_latency_hint(old_latency_hint);
> >
> > return rc;
> > ===================
> >
> >
> > >
> > > Cheers,
> > > Ben.
> > >
> > > > [ 56.618846] WARNING: at arch/powerpc/kernel/irq.c:240
> > > > [ 56.618851] Modules linked in: rcutorture ipv6 dm_mod ext3 jbd mbcache sg sd_mod crc_t10dif ibmvscsic scsi_transport_srp scsi_tgt ibmveth
> > > > [ 56.618883] NIP: c00000000000ff94 LR: c00000000067a5e0 CTR: 0000000000000001
> > > > [ 56.618889] REGS: c0000001fef6bbe0 TRAP: 0700 Not tainted (3.6.0-rc1-autokern1)
> > > > [ 56.618894] MSR: 8000000000029032 <SF,EE,ME,IR,DR,RI> CR: 42000082 XER: 20000000
> > > > [ 56.618913] SOFTE: 1
> > > > [ 56.618916] CFAR: c00000000067a5dc
> > > > [ 56.618920] TASK = c0000001feed79a0[0] 'swapper/5' THREAD: c0000001fef68000 CPU: 5
> > > > GPR00: 0000000000000001 c0000001fef6be60 c000000000f9ca08 0000000000000001
> > > > GPR04: 0000000000000001 0000000000000008 0000000000000001 0000000000000000
> > > > GPR08: 0000000000000000 c0000001feed79a0 0008a80000000000 0000000000000000
> > > > GPR12: 0000000022000082 c00000000f330f00 c0000001fef6bf90 000000000f394b4c
> > > > GPR16: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
> > > > GPR20: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
> > > > GPR24: c000000000fe8f80 0000000000000008 0000000000000028 0000000000000000
> > > > GPR28: 0000000000000000 0000000000000020 c000000000f1ab40 0000000000000001
> > > > [ 56.619014] NIP [c00000000000ff94] .arch_local_irq_restore+0x34/0xa0
> > > > [ 56.619020] LR [c00000000067a5e0] .start_secondary+0x368/0x37c
> > > > [ 56.619025] Call Trace:
> > > > [ 56.619030] [c0000001fef6be60] [c000000001ba0500] 0xc000000001ba0500 (unreliable)
> > > > [ 56.619038] [c0000001fef6bed0] [c00000000067a5e0] .start_secondary+0x368/0x37c
> > > > [ 56.619046] [c0000001fef6bf90] [c000000000009380] .start_secondary_resume+0x10/0x14
> > > > [ 56.619052] Instruction dump:
> > > > [ 56.619056] f8010010 f821ff91 986d022a 2fa30000 419e0054 880d022b 78000621 41820048
> > > > [ 56.619071] 2f800001 40de0064 7c0000a6 78008fe2 <0b000000> 2fa00000 40de0050 38000000
> > > > [ 56.619088] ---[ end trace 0199c0d783d7f9ba ]---
> > > >
> > > > Reported-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> > > > Signed-off-by: Li Zhong <zhong@linux.vnet.ibm.com>
> > > > ---
> > > > arch/powerpc/platforms/pseries/hotplug-cpu.c | 2 ++
> > > > 1 files changed, 2 insertions(+), 0 deletions(-)
> > > >
> > > > diff --git a/arch/powerpc/platforms/pseries/hotplug-cpu.c b/arch/powerpc/platforms/pseries/hotplug-cpu.c
> > > > index 64c97d8..8de539a 100644
> > > > --- a/arch/powerpc/platforms/pseries/hotplug-cpu.c
> > > > +++ b/arch/powerpc/platforms/pseries/hotplug-cpu.c
> > > > @@ -137,6 +137,8 @@ static void pseries_mach_cpu_die(void)
> > > > if (get_preferred_offline_state(cpu) == CPU_STATE_ONLINE) {
> > > > unregister_slb_shadow(hwcpu);
> > > >
> > > > + __hard_irq_disable();
> > > > +
> > > > /*
> > > > * Call to start_secondary_resume() will not return.
> > > > * Kernel stack will be reset and start_secondary()
> > >
> > >
> >
>
>
^ permalink raw reply
* [PATCH] TTY: hvcs: fix missing unlock on error in hvcs_initialize()
From: Wei Yongjun @ 2012-10-22 4:42 UTC (permalink / raw)
To: gregkh; +Cc: yongjun_wei, linuxppc-dev, linux-kernel
From: Wei Yongjun <yongjun_wei@trendmicro.com.cn>
Add the missing unlock on the error handling path in function
hvcs_initialize().
Signed-off-by: Wei Yongjun <yongjun_wei@trendmicro.com.cn>
---
drivers/tty/hvc/hvcs.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/tty/hvc/hvcs.c b/drivers/tty/hvc/hvcs.c
index cab5c7a..744c3b8 100644
--- a/drivers/tty/hvc/hvcs.c
+++ b/drivers/tty/hvc/hvcs.c
@@ -1496,8 +1496,10 @@ static int __devinit hvcs_initialize(void)
num_ttys_to_alloc = hvcs_parm_num_devs;
hvcs_tty_driver = alloc_tty_driver(num_ttys_to_alloc);
- if (!hvcs_tty_driver)
+ if (!hvcs_tty_driver) {
+ mutex_unlock(&hvcs_init_mutex);
return -ENOMEM;
+ }
if (hvcs_alloc_index_list(num_ttys_to_alloc)) {
rc = -ENOMEM;
^ permalink raw reply related
* RE: [PATCH 1/1] usb: gadget: Don't attempt to dequeue requests for a disabled USB endpoint on Freescale hardware
From: Li Yang-R58472 @ 2012-10-22 3:33 UTC (permalink / raw)
To: balbi@ti.com, Simon Haggett
Cc: Greg Kroah-Hartman, linux-usb@vger.kernel.org,
linuxppc-dev@lists.ozlabs.org, linux-kernel@vger.kernel.org,
Laurent Pinchart
In-Reply-To: <20121019173718.GA2738@arwen.pp.htv.fi>
> -----Original Message-----
> From: Felipe Balbi [mailto:balbi@ti.com]
> Sent: Saturday, October 20, 2012 1:37 AM
> To: Simon Haggett
> Cc: Li Yang-R58472; Felipe Balbi; Greg Kroah-Hartman; linux-
> usb@vger.kernel.org; linuxppc-dev@lists.ozlabs.org; linux-
> kernel@vger.kernel.org; Laurent Pinchart
> Subject: Re: [PATCH 1/1] usb: gadget: Don't attempt to dequeue requests
> for a disabled USB endpoint on Freescale hardware
>=20
> Hi,
>=20
> On Fri, Oct 19, 2012 at 06:19:26PM +0100, Simon Haggett wrote:
> > Some gadget drivers may attempt to dequeue requests for an endpoint
> > that has already been disabled. For example, in the UVC gadget driver,
> > uvc_function_set_alt() will call usb_ep_disable() when alt setting 0
> > is selected. When the userspace application subsequently issues the
> > VIDIOC_STREAMOFF ioctl, uvc_video_enable() invokes usb_ep_dequeue() to
> ensure that all requests have been cancelled.
>=20
> bug is on uvc gadget, then. Laurent ?
>=20
> Also, fsl should be removed from the tree, I'm trying to persuade iMX
> folks to use drivers/usb/chipidea instead.
Besides the iMX usage, the driver is also being used by many Freescale Powe=
rPC/Coldfire SoCs. I agree that it's ideal to move to a common driver. Bu=
t it is a large task to make the chipidea driver works for all the hardware=
that fsl_udc had supported and been tested on.
>=20
> > For the Freescale High Speed Dual-Role USB controller,
> > fsl_ep_dequeue() provides the implementation of usb_ep_dequeue(). If
> > this is called for a disabled endpoint, a kernel oops will occur when
> the ep->ep.desc field is dereferenced (by ep_index()).
> > fsl_ep_disable() sets this field to NULL, as well as deleting all
> > pending requests for the endpoint.
> >
> > This patch adds an additional check to fsl_ep_dequeue() to ensure that
> > the endpoint has not already been disabled before attempting to dequeue
> requests.
> >
> > Signed-off-by: Simon Haggett <simon.haggett@realvnc.com>
> > ---
> > drivers/usb/gadget/fsl_udc_core.c | 5 ++++-
> > 1 files changed, 4 insertions(+), 1 deletions(-)
> >
> > diff --git a/drivers/usb/gadget/fsl_udc_core.c
> > b/drivers/usb/gadget/fsl_udc_core.c
> > index 6ae70cb..acd513b 100644
> > --- a/drivers/usb/gadget/fsl_udc_core.c
> > +++ b/drivers/usb/gadget/fsl_udc_core.c
> > @@ -955,7 +955,10 @@ static int fsl_ep_dequeue(struct usb_ep *_ep,
> struct usb_request *_req)
> > int ep_num, stopped, ret =3D 0;
> > u32 epctrl;
> >
> > - if (!_ep || !_req)
> > + /* Ensure that the ep and request are valid, and the ep is not
> > + * disabled
> > + */
> > + if (!_ep || !_req || !ep->ep.desc)
> > return -EINVAL;
> >
> > spin_lock_irqsave(&ep->udc->lock, flags);
> > --
> > 1.7.4.1
> >
>=20
> --
> balbi
^ permalink raw reply
* [PATCH] of/fdt: Don't copy garbage after "/" in root node path
From: Benjamin Herrenschmidt @ 2012-10-22 0:32 UTC (permalink / raw)
To: devicetree-discuss; +Cc: linuxppc-dev
The root node path must be internally converted to "/", or various
pieces of code looking for it that way will fail. The code to do
that however had a bug where we might incorrectly append pieces
of the original path from the fdt to the "/".
We should probably add a proper dedicated accessor for the root node
but in the meantime this patch should fix it.
Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
drivers/of/fdt.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
index 91a375f..c2b08dc 100644
--- a/drivers/of/fdt.c
+++ b/drivers/of/fdt.c
@@ -186,6 +186,7 @@ static unsigned long unflatten_dt_node(struct boot_param_header *blob,
*/
fpsize = 1;
allocl = 2;
+ l = 0;
} else {
/* account for '/' and path size minus terminal 0
* already in 'l'
^ permalink raw reply related
* [PATCH] powerpc/powernv: Fix OPAL debug entry
From: Benjamin Herrenschmidt @ 2012-10-22 0:30 UTC (permalink / raw)
To: linuxppc-dev
OPAL provides the firmware base/entry in registers at boot time
for debugging purposes. We had a bug in the code trying to stash
these into the appropriate kernel globals (a line of code was
probably dropped by accident back when this was merged)
Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
arch/powerpc/kernel/head_64.S | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/powerpc/kernel/head_64.S b/arch/powerpc/kernel/head_64.S
index 58bddee..694e3fa 100644
--- a/arch/powerpc/kernel/head_64.S
+++ b/arch/powerpc/kernel/head_64.S
@@ -703,6 +703,7 @@ _INIT_STATIC(start_here_multiplatform)
#ifdef CONFIG_PPC_EARLY_DEBUG_OPAL
/* Setup OPAL entry */
+ LOAD_REG_ADDR(r11, opal)
std r28,0(r11);
std r29,8(r11);
#endif
^ permalink raw reply related
* Re: ELDK 4.2/kilauea/3.5+ kernel broken
From: Benjamin Herrenschmidt @ 2012-10-21 20:01 UTC (permalink / raw)
To: Robert Berger; +Cc: linuxppc-dev, wd, mla
In-Reply-To: <50841653.4050301@reliableembeddedsystems.com>
On Sun, 2012-10-21 at 18:35 +0300, Robert Berger wrote:
> Hi,
>
> On 10/18/2012 10:03 PM, Benjamin Herrenschmidt wrote:
> >
> > Which looks correct. So this might be something specific to ELDK ?
>
> I just tried with the ELDK 5.2.1 and have exactly the same behavior as
> with ELDK 4.2, so I guess there is something not correct in
> arch/powerpc/sysdev/ppc4xx_msi.c.
>
> Just to exclude the case that I'm doing something stupid which only
> affects kernel versions 3.5+ and not 3.4 can someone else give it a try
> with a kilauea board or something similar?
Remind me what is the symptom ? A specific device isn't working ? Or the
whole kernel goes toast ? My feeling is that those patches make MSIs
work (well that's what they are supposed to do) and for some reason that
doesn't agree with whatever you have connected to the PCIe slot...
Cheers,
Ben.
> Regards,
>
> Robert
>
> >
> > Cheers,
> > Ben.
> >
>
> ..."Serious programming is systems programming or anything beyond
> writing factorial(n) as a student exercise." -- Brian W. Kernighan
>
> My public pgp key is available,at:
> http://pgp.mit.edu:11371/pks/lookup?op=get&search=0x90320BF1
^ permalink raw reply
* Re: ELDK 4.2/kilauea/3.5+ kernel broken
From: Robert Berger @ 2012-10-21 15:35 UTC (permalink / raw)
To: Benjamin Herrenschmidt; +Cc: linuxppc-dev, wd, mla
In-Reply-To: <1350587006.2476.12.camel__24873.815305955$1350587085$gmane$org@pasglop>
Hi,
On 10/18/2012 10:03 PM, Benjamin Herrenschmidt wrote:
>
> Which looks correct. So this might be something specific to ELDK ?
I just tried with the ELDK 5.2.1 and have exactly the same behavior as
with ELDK 4.2, so I guess there is something not correct in
arch/powerpc/sysdev/ppc4xx_msi.c.
Just to exclude the case that I'm doing something stupid which only
affects kernel versions 3.5+ and not 3.4 can someone else give it a try
with a kilauea board or something similar?
Regards,
Robert
>
> Cheers,
> Ben.
>
..."Serious programming is systems programming or anything beyond
writing factorial(n) as a student exercise." -- Brian W. Kernighan
My public pgp key is available,at:
http://pgp.mit.edu:11371/pks/lookup?op=get&search=0x90320BF1
^ permalink raw reply
* Re: ELDK 4.2/kilauea/3.5+ kernel broken
From: Robert Berger @ 2012-10-21 15:39 UTC (permalink / raw)
To: linuxppc-dev
In-Reply-To: <1350587006.2476.12.camel__24873.815305955$1350587085$gmane$org@pasglop>
Hi,
On 10/18/2012 10:03 PM, Benjamin Herrenschmidt wrote:
>
> Which looks correct. So this might be something specific to ELDK ?
I just tried with the ELDK 5.2.1 and have exactly the same behavior as
with ELDK 4.2, so I guess there is something not correct in
arch/powerpc/sysdev/ppc4xx_msi.c.
Just to exclude the case that I'm doing something stupid which only
affects kernel versions 3.5+ and not 3.4 can someone else give it a try
with a kilauea board or something similar?
Regards,
Robert
>
> Cheers,
> Ben.
>
^ permalink raw reply
* [PATCH 3/5] arch/powerpc/kernel/rtas_flash.c: eliminate possible double free
From: Julia Lawall @ 2012-10-21 10:52 UTC (permalink / raw)
To: Benjamin Herrenschmidt
Cc: Paul Mackerras, kernel-janitors, linuxppc-dev, linux-kernel
In-Reply-To: <1350816727-1381-1-git-send-email-Julia.Lawall@lip6.fr>
From: Julia Lawall <Julia.Lawall@lip6.fr>
The function initialize_flash_pde_data is only called four times. All four
calls are in the function rtas_flash_init, and on the failure of any of the
calls, remove_flash_pde is called on the third argument of each of the
calls. There is thus no need for initialize_flash_pde_data to call
remove_flash_pde on the same argument. remove_flash_pde kfrees the data
field of its argument, and does not clear that field, so this amounts ot a
possible double free.
A simplified version of the semantic match that finds this problem is as
follows: (http://coccinelle.lip6.fr/)
// <smpl>
@r@
identifier f,free,a;
parameter list[n] ps;
type T;
expression e;
@@
f(ps,T a,...) {
... when any
when != a = e
if(...) { ... free(a); ... return ...; }
... when any
}
@@
identifier r.f,r.free;
expression x,a;
expression list[r.n] xs;
@@
* x = f(xs,a,...);
if (...) { ... free(a); ... return ...; }
// </smpl>
Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>
---
Not tested.
arch/powerpc/kernel/rtas_flash.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/arch/powerpc/kernel/rtas_flash.c b/arch/powerpc/kernel/rtas_flash.c
index 20b0120..8329190 100644
--- a/arch/powerpc/kernel/rtas_flash.c
+++ b/arch/powerpc/kernel/rtas_flash.c
@@ -650,10 +650,8 @@ static int initialize_flash_pde_data(const char *rtas_call_name,
int token;
dp->data = kzalloc(buf_size, GFP_KERNEL);
- if (dp->data == NULL) {
- remove_flash_pde(dp);
+ if (dp->data == NULL)
return -ENOMEM;
- }
/*
* This code assumes that the status int is the first member of the
^ permalink raw reply related
* Re: PowerMac PMU programming
From: Justin Hibbits @ 2012-10-21 2:56 UTC (permalink / raw)
To: Benjamin Herrenschmidt; +Cc: linuxppc-dev
In-Reply-To: <1339449862.9220.13.camel@pasglop>
Hi Ben,
Looking at this again, so bringing up this thread, although it is 4
months old.
On Tue, 12 Jun 2012 07:24:22 +1000
Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:
> On Sun, 2012-06-10 at 23:31 -0400, Justin Hibbits wrote:
>=20
> > I'll settle on just getting the CPU speed change working, losing
> > 333MHz on my TiBook is a few too many MHz for the poor thing to
> > lose :) I do have a desktop, but that's my development platform,
> > so needs to be at least somewhat stable, so my TiBook is my guinea
> > pig.
> >=20
> > I've googled everything I can think of, but do you know of any
> > definitive PMU documentation? Or can you give me any hints on
> > what's needed just for that simple operation?
>=20
> No doco no, Darwin and Linux source code are your best bet.
>=20
> It works like sleep as in: you send the switch command and the PMU
> will reboot the processor with the new frequency whenever the
> processor enters NAP mode.
>=20
> This is tricky because there's a loss of cache coherency vs. ongoing
> DMAs, hence tricky bits of code in there to deal with the various
> caches.
>=20
> > I noticed that you wind down the ADB queue before killing the PMU.
> > Is this necessary? I only disable the interrupt by writing 0x10 to
> > the IER, but our interrupt model for the PMU is different from the
> > Linux one.=20
>=20
> Well that will not disable CB1 (or the external GPIO one). You need to
> make sure no other communication with the PMU takes place once you've
> sent the sleep command and until you're back.
>=20
> > Is it also necessary to disable the interrupt input, even though
> > the register write should be disabling the PMU's asserting of the
> > line? There doesn't seem to be much more different than that, and
> > setting the openpic priority, Linux sets it to 0x0f but I didn't see
> > anything in OS X doing that (might that be a problem, too?)
> >=20
> > I would love to get my TiBook using the full 1GHz.
>=20
> I don't remember what openpic does in Darwin, I think interrupts are
> all masked there as a side effect of the code in KeyLargo.cpp but I'm
> not sure that applies to frequency changes.
>=20
> Cheers,
> Ben.
=46rom looking at the Linux PMU source, and the FreeBSD source, I can see
only cosmetic differences. However, the biggest thing I see between
the Linux PPC code and the FreeBSD code is that Linux quiesces Open
Firmware right away. I haven't tried this with FreeBSD yet, but from
your experience with development on this platform, would you think that
not quiescing OFW might cause the catatonic state?
- Justin
^ permalink raw reply
* Re: [PATCH v3 0/7] mv643xx.c: Add basic device tree support.
From: Jason Cooper @ 2012-10-21 1:52 UTC (permalink / raw)
To: Benjamin Herrenschmidt
Cc: thomas.petazzoni, andrew, Arnd Bergmann, netdev,
devicetree-discuss, ben.dooks, Ian Molton, dale, linuxppc-dev,
David Miller, linux-arm-kernel
In-Reply-To: <1347343411.2603.42.camel@pasglop>
Pong. ;-)
On Tue, Sep 11, 2012 at 04:03:31PM +1000, Benjamin Herrenschmidt wrote:
> On Mon, 2012-09-10 at 14:22 +0000, Arnd Bergmann wrote:
> > Following up on the old discussion, I talked briefly about this
> > issue with BenH at the kernel summit. The outcome basically is that
> > it's a bit sad to have incompatible bindings, but it's not the end
> > of the world,and it's more important to do it right this time.
> >
> > Just make sure that you use different values for the 'compatible'
> > strings and then do what you need to get the ARM hardware working.
> >
> > Ideally, the new binding should be written in a way that powerpc
> > machines can use the same one, but the existing ones all use
> > an version of Open Firmware that is not going to get updated
> > and it's also not too likely that we are going to see new
> > powerpc machines based on this chip.
>
> Right, mostly these machines where the Pegasos. Those came with a fairly
> busted variant of Open Firmware which generated a pretty gross
> device-tree.
>
> For some reason, the manufacturer of those things was never willing to
> fix anything in their firmware (despite the distributor providing
> patches etc...), seemingly on the assumption that whatever they were
> doing was perfect and operating system people like us didn't matter one
> little bit :-)
>
> So I don't care much about it. It would be nice to keep them working
> since people in the community still have them but if it goes through
> some "compat" code that detects old/broken device-trees and eventually
> disappears when we finally drop support, then so be it.
Ian,
What is the status of this work?
thx,
Jason.
^ permalink raw reply
* Re: [PATCH 5/7] usb: gadget: fsl_udc: postpone freeing current dTD
From: Christoph Fritz @ 2012-10-20 19:12 UTC (permalink / raw)
To: balbi
Cc: Estevam Fabio-R49496, Li Yang-R58472, Greg Kroah-Hartman,
Chen Peter-B29397, Sascha Hauer, linux-usb, Hans J. Koch,
Daniel Mack, Christian Hemp, linuxppc-dev, Teresa Gamez,
Sebastian Andrzej Siewior
In-Reply-To: <20121019104416.GH11191@arwen.pp.htv.fi>
On Fri, 2012-10-19 at 13:44 +0300, Felipe Balbi wrote:
> > I thought about this too but wasn't able to use chipidea with
> > MXC_EHCI_INTERNAL_PHY as it's called in fsl_udc.
>
> that's a matter of writing the PHY driver, right ;-) It has nothing to
> do with chipidea, actually :-)
Okay, I'll do. But then we should purge the old buggy fsl_udc.
Thanks
-- Christoph
^ permalink raw reply
* Re: [PATCH v2] powerpc: 52xx: nop out unsupported critical IRQs
From: Anatolij Gustschin @ 2012-10-20 11:59 UTC (permalink / raw)
To: Wolfram Sang; +Cc: John Bonesio, linuxppc-dev
In-Reply-To: <1349947155-11540-1-git-send-email-w.sang@pengutronix.de>
Hi,
On Thu, 11 Oct 2012 11:19:15 +0200
Wolfram Sang <w.sang@pengutronix.de> wrote:
> Currently, when booting MPC52xx based platforms, we get:
>
> mpc52xx_irqhost_map: invalid irq: virq=16, l1=0, l2=3
> irq: irq-16==>hwirq-0x3 mapping failed: -22
> [WARNing skipped]
>
> The warning is wrong since the mapping itself is valid. However, there is no
> support for that type of IRQ currently. Print a proper warning and bind the irq
> to a no_irq chip.
>
> Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>
> Cc: John Bonesio <bones@secretlab.ca>
> Cc: Anatolij Gustschin <agust@denx.de>
> Cc: Grant Likely <grant.likely@secretlab.ca>
> ---
>
> Change since V1: drop the default state since all possibilities for l1 are now
> covered in the switch-statement.
>
> arch/powerpc/platforms/52xx/mpc52xx_pic.c | 9 +++++----
applied, thanks!
Anatolij
^ permalink raw reply
* Re: [PATCH v2] powerpc: 52xx: nop out unsupported critical IRQs
From: Wolfram Sang @ 2012-10-20 6:54 UTC (permalink / raw)
To: linuxppc-dev; +Cc: John Bonesio, Anatolij Gustschin
In-Reply-To: <1349947155-11540-1-git-send-email-w.sang@pengutronix.de>
[-- Attachment #1: Type: text/plain, Size: 2180 bytes --]
On Thu, Oct 11, 2012 at 11:19:15AM +0200, Wolfram Sang wrote:
> Currently, when booting MPC52xx based platforms, we get:
>
> mpc52xx_irqhost_map: invalid irq: virq=16, l1=0, l2=3
> irq: irq-16==>hwirq-0x3 mapping failed: -22
> [WARNing skipped]
>
> The warning is wrong since the mapping itself is valid. However, there is no
> support for that type of IRQ currently. Print a proper warning and bind the irq
> to a no_irq chip.
>
> Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>
> Cc: John Bonesio <bones@secretlab.ca>
> Cc: Anatolij Gustschin <agust@denx.de>
> Cc: Grant Likely <grant.likely@secretlab.ca>
Ping. I'd really like to get rid of the WARNing for all MPC52xx boards.
It looks pretty scary to people who don't know the issues behind it.
> ---
>
> Change since V1: drop the default state since all possibilities for l1 are now
> covered in the switch-statement.
>
> arch/powerpc/platforms/52xx/mpc52xx_pic.c | 9 +++++----
> 1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/arch/powerpc/platforms/52xx/mpc52xx_pic.c b/arch/powerpc/platforms/52xx/mpc52xx_pic.c
> index 8520b58..b89ef65 100644
> --- a/arch/powerpc/platforms/52xx/mpc52xx_pic.c
> +++ b/arch/powerpc/platforms/52xx/mpc52xx_pic.c
> @@ -372,10 +372,11 @@ static int mpc52xx_irqhost_map(struct irq_domain *h, unsigned int virq,
> case MPC52xx_IRQ_L1_MAIN: irqchip = &mpc52xx_main_irqchip; break;
> case MPC52xx_IRQ_L1_PERP: irqchip = &mpc52xx_periph_irqchip; break;
> case MPC52xx_IRQ_L1_SDMA: irqchip = &mpc52xx_sdma_irqchip; break;
> - default:
> - pr_err("%s: invalid irq: virq=%i, l1=%i, l2=%i\n",
> - __func__, virq, l1irq, l2irq);
> - return -EINVAL;
> + case MPC52xx_IRQ_L1_CRIT:
> + pr_warn("%s: Critical IRQ #%d is unsupported! Nopping it.\n",
> + __func__, l2irq);
> + irq_set_chip(virq, &no_irq_chip);
> + return 0;
> }
>
> irq_set_chip_and_handler(virq, irqchip, handle_level_irq);
> --
> 1.7.10.4
>
--
Pengutronix e.K. | Wolfram Sang |
Industrial Linux Solutions | http://www.pengutronix.de/ |
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]
^ permalink raw reply
* Re: [PATCH 1/10] memory-hotplug : check whether memory is offline or not when removing memory
From: Wen Congyang @ 2012-10-20 0:50 UTC (permalink / raw)
To: KOSAKI Motohiro
Cc: linux-s390, linux-ia64, len.brown, linux-acpi, linux-sh, x86,
linux-kernel, cmetcalf, Wen Congyang, linux-mm, Yasuaki Ishimatsu,
minchan.kim, rientjes, sparclinux, cl, linuxppc-dev, akpm, liuj97
In-Reply-To: <CAHGf_=q=Agidyj_j6jhBdhNmJBy2u1dP+UMAoXbM=_=DyZJs_w@mail.gmail.com>
At 10/20/2012 02:33 AM, KOSAKI Motohiro Wrote:
>> I think it again, and found that this check is necessary. Because we only
>> lock memory hotplug when offlining pages. Here is the steps to offline and
>> remove memory:
>>
>> 1. lock memory hotplug
>> 2. offline a memory section
>> 3. unlock memory hotplug
>> 4. repeat 1-3 to offline all memory sections
>> 5. lock memory hotplug
>> 6. remove memory
>> 7. unlock memory hotplug
>>
>> All memory sections must be offlined before removing memory. But we don't
>> hold
>> the lock in the whole operation. So we should check whether all memory
>> sections
>> are offlined before step6.
>
> You should describe the race scenario in the patch description. OK?
>
OK
Thanks
Wen Congyang
^ permalink raw reply
* Re: [PATCH 1/10] memory-hotplug : check whether memory is offline or not when removing memory
From: KOSAKI Motohiro @ 2012-10-19 18:33 UTC (permalink / raw)
To: Wen Congyang
Cc: linux-s390, linux-ia64, Wen Congyang, linux-acpi, linux-sh,
len.brown, x86, linux-kernel, cmetcalf, linux-mm,
Yasuaki Ishimatsu, minchan.kim, rientjes, sparclinux, cl,
linuxppc-dev, akpm, liuj97
In-Reply-To: <5081609C.9080702@gmail.com>
> I think it again, and found that this check is necessary. Because we only
> lock memory hotplug when offlining pages. Here is the steps to offline and
> remove memory:
>
> 1. lock memory hotplug
> 2. offline a memory section
> 3. unlock memory hotplug
> 4. repeat 1-3 to offline all memory sections
> 5. lock memory hotplug
> 6. remove memory
> 7. unlock memory hotplug
>
> All memory sections must be offlined before removing memory. But we don't
> hold
> the lock in the whole operation. So we should check whether all memory
> sections
> are offlined before step6.
You should describe the race scenario in the patch description. OK?
^ permalink raw reply
* [PATCH 1/1] usb: gadget: Don't attempt to dequeue requests for a disabled USB endpoint on Freescale hardware
From: Simon Haggett @ 2012-10-19 17:19 UTC (permalink / raw)
To: Li Yang, Felipe Balbi
Cc: Simon Haggett, Greg Kroah-Hartman, linux-usb, linuxppc-dev,
linux-kernel
Some gadget drivers may attempt to dequeue requests for an endpoint that has
already been disabled. For example, in the UVC gadget driver, uvc_function_set_alt()
will call usb_ep_disable() when alt setting 0 is selected. When the userspace
application subsequently issues the VIDIOC_STREAMOFF ioctl, uvc_video_enable()
invokes usb_ep_dequeue() to ensure that all requests have been cancelled.
For the Freescale High Speed Dual-Role USB controller, fsl_ep_dequeue() provides
the implementation of usb_ep_dequeue(). If this is called for a disabled endpoint,
a kernel oops will occur when the ep->ep.desc field is dereferenced (by ep_index()).
fsl_ep_disable() sets this field to NULL, as well as deleting all pending requests
for the endpoint.
This patch adds an additional check to fsl_ep_dequeue() to ensure that the
endpoint has not already been disabled before attempting to dequeue requests.
Signed-off-by: Simon Haggett <simon.haggett@realvnc.com>
---
drivers/usb/gadget/fsl_udc_core.c | 5 ++++-
1 files changed, 4 insertions(+), 1 deletions(-)
diff --git a/drivers/usb/gadget/fsl_udc_core.c b/drivers/usb/gadget/fsl_udc_core.c
index 6ae70cb..acd513b 100644
--- a/drivers/usb/gadget/fsl_udc_core.c
+++ b/drivers/usb/gadget/fsl_udc_core.c
@@ -955,7 +955,10 @@ static int fsl_ep_dequeue(struct usb_ep *_ep, struct usb_request *_req)
int ep_num, stopped, ret = 0;
u32 epctrl;
- if (!_ep || !_req)
+ /* Ensure that the ep and request are valid, and the ep is not
+ * disabled
+ */
+ if (!_ep || !_req || !ep->ep.desc)
return -EINVAL;
spin_lock_irqsave(&ep->udc->lock, flags);
--
1.7.4.1
^ permalink raw reply related
* Re: [PATCH 1/1] usb: gadget: Don't attempt to dequeue requests for a disabled USB endpoint on Freescale hardware
From: Felipe Balbi @ 2012-10-19 17:37 UTC (permalink / raw)
To: Simon Haggett
Cc: Greg Kroah-Hartman, linux-usb, linux-kernel, Felipe Balbi,
Laurent Pinchart, linuxppc-dev
In-Reply-To: <1350667166-3559-1-git-send-email-simon.haggett@realvnc.com>
[-- Attachment #1: Type: text/plain, Size: 1972 bytes --]
Hi,
On Fri, Oct 19, 2012 at 06:19:26PM +0100, Simon Haggett wrote:
> Some gadget drivers may attempt to dequeue requests for an endpoint that has
> already been disabled. For example, in the UVC gadget driver, uvc_function_set_alt()
> will call usb_ep_disable() when alt setting 0 is selected. When the userspace
> application subsequently issues the VIDIOC_STREAMOFF ioctl, uvc_video_enable()
> invokes usb_ep_dequeue() to ensure that all requests have been cancelled.
bug is on uvc gadget, then. Laurent ?
Also, fsl should be removed from the tree, I'm trying to persuade iMX
folks to use drivers/usb/chipidea instead.
> For the Freescale High Speed Dual-Role USB controller, fsl_ep_dequeue() provides
> the implementation of usb_ep_dequeue(). If this is called for a disabled endpoint,
> a kernel oops will occur when the ep->ep.desc field is dereferenced (by ep_index()).
> fsl_ep_disable() sets this field to NULL, as well as deleting all pending requests
> for the endpoint.
>
> This patch adds an additional check to fsl_ep_dequeue() to ensure that the
> endpoint has not already been disabled before attempting to dequeue requests.
>
> Signed-off-by: Simon Haggett <simon.haggett@realvnc.com>
> ---
> drivers/usb/gadget/fsl_udc_core.c | 5 ++++-
> 1 files changed, 4 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/usb/gadget/fsl_udc_core.c b/drivers/usb/gadget/fsl_udc_core.c
> index 6ae70cb..acd513b 100644
> --- a/drivers/usb/gadget/fsl_udc_core.c
> +++ b/drivers/usb/gadget/fsl_udc_core.c
> @@ -955,7 +955,10 @@ static int fsl_ep_dequeue(struct usb_ep *_ep, struct usb_request *_req)
> int ep_num, stopped, ret = 0;
> u32 epctrl;
>
> - if (!_ep || !_req)
> + /* Ensure that the ep and request are valid, and the ep is not
> + * disabled
> + */
> + if (!_ep || !_req || !ep->ep.desc)
> return -EINVAL;
>
> spin_lock_irqsave(&ep->udc->lock, flags);
> --
> 1.7.4.1
>
--
balbi
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [RFC][PATCH] perf: Add a few generic stalled-cycles events
From: Sukadev Bhattiprolu @ 2012-10-19 17:05 UTC (permalink / raw)
To: Stephane Eranian
Cc: linuxppc-dev, Robert Richter, Anton Blanchard, LKML,
Arnaldo Carvalho de Melo, Peter Zijlstra, Paul Mackerras, mpjohn,
Ingo Molnar, Arun Sharma
In-Reply-To: <CABPqkBRd2wfjd1ObCkM+TSSwt7C9WP65dEE9POSyDK1ccvNL8A@mail.gmail.com>
Stephane Eranian [eranian@google.com] wrote:
| So all in all, I think this is not a very good idea. You have to put
| this into the tool or a library that auto-detects the
| host CPU and programs the right set of events.
|
| We've had that discussion many times. Just reiterating my personal
| opinion on this.
Yes that would work too. One drawback is that the hardware events
will be in the tool, while the software/tracepoint events in the
kernel sysfs representation.
Or is that the reason we want all events in one place (sysfs) ?
Sukadev
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox