Re: [PATCH] staging: rtl8723bs: Fix possible buffer overrun
Yes, you are right. I will send a new patch.
Young
On 2018/11/28 14:51, Dan Carpenter wrote:
> The original code is OK.
>
> On Wed, Nov 28, 2018 at 02:22:31AM +, Yang Xiao wrote:
>> From: Young Xiao
>>
>> In routine rtw_report_sec_ie(), the code could set the length
>> of the buffer to 256; however, that value is one larger than the
>> corresponding memory allocation.
>>
>> See commit 8b7a13c3f404 ("staging: r8712u: Fix possible
>> buffer overrun") for detail.
> This bug is from 2012... It's a real bug, but looking at things in
> retrospect we probably didn't do the right fix. The correct patch would
> be to revert 8b7a13c3f404 and change this instead:
>
> Can you send that? Do it as one patch. (Don't make it a revert commit,
> that's just a headache, make it a normal patch with a Fixes tag). The
> commit message would look something like:
>
>In commit 8b7a13c3f404 ("staging: r8712u: Fix possible buffer
>overrun") we fix a potential off by one by making the limit smaller.
>The better fix is to make the buffer larger. This makes it match up
>with the similar code in other drivers. Blah blah blah. Etc.
>
> diff --git a/drivers/staging/rtl8712/rtl871x_mlme.c
> b/drivers/staging/rtl8712/rtl871x_mlme.c
> index a7374006a9fb..986a1d526918 100644
> --- a/drivers/staging/rtl8712/rtl871x_mlme.c
> +++ b/drivers/staging/rtl8712/rtl871x_mlme.c
> @@ -1346,7 +1346,7 @@ sint r8712_restruct_sec_ie(struct _adapter *adapter, u8
> *in_ie,
>u8 *out_ie, uint in_len)
> {
> u8 authmode = 0, match;
> - u8 sec_ie[255], uncst_oui[4], bkup_ie[255];
> + u8 sec_ie[IW_CUSTOM_MAX], uncst_oui[4], bkup_ie[255];
> u8 wpa_oui[4] = {0x0, 0x50, 0xf2, 0x01};
> uint ielength, cnt, remove_cnt;
> int iEntry;
>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: rtl8723bs: Fix possible buffer overrun
The original code is OK.
On Wed, Nov 28, 2018 at 02:22:31AM +, Yang Xiao wrote:
> From: Young Xiao
>
> In routine rtw_report_sec_ie(), the code could set the length
> of the buffer to 256; however, that value is one larger than the
> corresponding memory allocation.
>
> See commit 8b7a13c3f404 ("staging: r8712u: Fix possible
> buffer overrun") for detail.
This bug is from 2012... It's a real bug, but looking at things in
retrospect we probably didn't do the right fix. The correct patch would
be to revert 8b7a13c3f404 and change this instead:
Can you send that? Do it as one patch. (Don't make it a revert commit,
that's just a headache, make it a normal patch with a Fixes tag). The
commit message would look something like:
In commit 8b7a13c3f404 ("staging: r8712u: Fix possible buffer
overrun") we fix a potential off by one by making the limit smaller.
The better fix is to make the buffer larger. This makes it match up
with the similar code in other drivers. Blah blah blah. Etc.
diff --git a/drivers/staging/rtl8712/rtl871x_mlme.c
b/drivers/staging/rtl8712/rtl871x_mlme.c
index a7374006a9fb..986a1d526918 100644
--- a/drivers/staging/rtl8712/rtl871x_mlme.c
+++ b/drivers/staging/rtl8712/rtl871x_mlme.c
@@ -1346,7 +1346,7 @@ sint r8712_restruct_sec_ie(struct _adapter *adapter, u8
*in_ie,
u8 *out_ie, uint in_len)
{
u8 authmode = 0, match;
- u8 sec_ie[255], uncst_oui[4], bkup_ie[255];
+ u8 sec_ie[IW_CUSTOM_MAX], uncst_oui[4], bkup_ie[255];
u8 wpa_oui[4] = {0x0, 0x50, 0xf2, 0x01};
uint ielength, cnt, remove_cnt;
int iEntry;
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [RFC PATCH v2] android: ion: How to properly clean caches for uncached allocations
On Tue, 27 Nov 2018, Brian Starkey wrote: > Hi Liam, > > On Mon, Nov 26, 2018 at 08:59:44PM -0800, Liam Mark wrote: > > On Tue, 20 Nov 2018, Brian Starkey wrote: > > > > > Hi Liam, > > > > > > I'm missing a bit of context here, but I did read the v1 thread. > > > Please accept my apologies if I'm re-treading trodden ground. > > > > > > I do know we're chasing nebulous ion "problems" on our end, which > > > certainly seem to be related to what you're trying to fix here. > > > > > > On Thu, Nov 01, 2018 at 03:15:06PM -0700, Liam Mark wrote: > > > >Based on the suggestions from Laura I created a first draft for a change > > > >which will attempt to ensure that uncached mappings are only applied to > > > >ION memory who's cache lines have been cleaned. > > > >It does this by providing cached mappings (for uncached ION allocations) > > > >until the ION buffer is dma mapped and successfully cleaned, then it > > > drops > > > >the userspace mappings and when pages are accessed they are faulted back > > > >in and uncached mappings are created. > > > > > > If I understand right, there's no way to portably clean the cache of > > > the kernel mapping before we map the pages into userspace. Is that > > > right? > > > > > > > Yes, it isn't always possible to clean the caches for an uncached mapping > > because a device is required by the DMA APIs to do cache maintenance and > > there isn't necessarily a device available (dma_buf_attach may not yet > > have been called). > > > > > Alternatively, can we just make ion refuse to give userspace a > > > non-cached mapping for pages which are mapped in the kernel as cached? > > > > These pages will all be mapped as cached in the kernel for 64 bit (kernel > > logical addresses) so you would always be refusing to create a non-cached > > mapping. > > And that might be the sane thing to do, no? > > AFAIK there are still pages which aren't ever mapped as cached (e.g. > dma_declare_coherent_memory(), anything under /reserved-memory marked > as no-map). If those are exposed as an ion heap, then non-cached > mappings would be fine, and permitted. > Sounds like you are suggesting using carveouts to support uncached? We have many multimedia use cases which use very large amounts of uncached memory, uncached memory is used as a performance optimization because CPU access won't happen so it allows us to skip cache maintenance for all the dma map and dma unmap calls. To create carveouts large enough to support to support the worst case scenarios could result in very large carveouts. Large carveouts like this would likely result in poor memory utilizations (since they are tuned for worst case) which would likely have significant performance impacts (more limited memory causes more frequent memory reclaim ect...). Also estimating for worst case could be difficult since the amount of uncached memory could be app dependent. Unfortunately I don't think this would make for a very scalable solution. > > > > > Would userspace using the dma-buf sync ioctl around its accesses do > > > the "right thing" in that case? > > > > > > > I don't think so, the dma-buf sync ioctl require a device to peform cache > > maintenance, but as mentioned above a device may not be available. > > > > If a device didn't attach yet, then no cache maintenance is > necessary. The only thing accessing the memory is the CPU, via a > cached mapping, which should work just fine. So far so good. > Unfortunately not. Scenario: - Client allocates uncached memory. - Client calls the DMA_BUF_IOCTL_SYNC IOCT IOCTL with flags DMA_BUF_SYNC_START (but this doesn't do any cache maintenance since there isn't any device) - Client mmap the memory (ION creates uncached mapping) - Client reads from that uncached mapping Because memory has not been cleaned (we haven't had a device yet) the zeros that were written to this memory could still be in the cache (since they were written with a cached mapping), this means that the unprivilived userpace client is now potentially reading sensitive kernel data > If there are already attachments, then ion_dma_buf_begin_cpu_access() > will sync for CPU access against all of the attached devices, and > again the CPU should see the right thing. > > In the other direction, ion_dma_buf_end_cpu_access() will sync for > device access for all currently attached devices. If there's no > attached devices yet, then there's nothing to do until there is (only > thing accessing is CPU via a CPU-cached mapping). > > When the first (or another) device attaches, then when it maps the > buffer, the map_dma_buf callback should do whatever sync-ing is needed > for that device. > > I might be way off with my understanding of the various DMA APIs, but > this is how I think they're meant to work. > > > > Given that as you pointed out, the kernel does still have a cached > > > mapping to these pages, trying to give the CPU a non-cached mapping of > > > those same pages while preserving
[PATCH] staging: rtl8723bs: Fix possible buffer overrun
From: Young Xiao
In routine rtw_report_sec_ie(), the code could set the length
of the buffer to 256; however, that value is one larger than the
corresponding memory allocation.
See commit 8b7a13c3f404 ("staging: r8712u: Fix possible
buffer overrun") for detail.
Signed-off-by: Young Xiao
---
drivers/staging/rtl8723bs/os_dep/mlme_linux.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/staging/rtl8723bs/os_dep/mlme_linux.c
b/drivers/staging/rtl8723bs/os_dep/mlme_linux.c
index da4bd52..085026c 100644
--- a/drivers/staging/rtl8723bs/os_dep/mlme_linux.c
+++ b/drivers/staging/rtl8723bs/os_dep/mlme_linux.c
@@ -165,7 +165,7 @@ void rtw_report_sec_ie(struct adapter *adapter, u8
authmode, u8 *sec_ie)
p += sprintf(p, "ASSOCINFO(ReqIEs =");
len = sec_ie[1] + 2;
- len = (len < IW_CUSTOM_MAX) ? len : IW_CUSTOM_MAX;
+ len = (len < IW_CUSTOM_MAX) ? len : IW_CUSTOM_MAX - 1;
for (i = 0; i < len; i++) {
p += sprintf(p, "%02x", sec_ie[i]);
--
2.7.4
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 2/2] staging: emxx_udc: Remove cast and move all in one line.
Remove the cast from IO_ADDRESS and use a single line.
Signed-off-by: Cristian Sicilia
---
As suggested by Dan we can just remove the cast and use an
unique line.
drivers/staging/emxx_udc/emxx_udc.c | 12
1 file changed, 4 insertions(+), 8 deletions(-)
diff --git a/drivers/staging/emxx_udc/emxx_udc.c
b/drivers/staging/emxx_udc/emxx_udc.c
index ebc622f..382683f 100644
--- a/drivers/staging/emxx_udc/emxx_udc.c
+++ b/drivers/staging/emxx_udc/emxx_udc.c
@@ -108,20 +108,16 @@ static void _nbu2ss_dump_register(struct nbu2ss_udc *udc)
dev_dbg(&udc->dev, "\n-USB REG-\n");
for (i = 0x0 ; i < USB_BASE_SIZE ; i += 16) {
- reg_data = _nbu2ss_readl(
- (u32 *)IO_ADDRESS(USB_BASE_ADDRESS + i));
+ reg_data = _nbu2ss_readl(IO_ADDRESS(USB_BASE_ADDRESS + i));
dev_dbg(&udc->dev, "USB%04x =%08x", i, (int)reg_data);
- reg_data = _nbu2ss_readl(
- (u32 *)IO_ADDRESS(USB_BASE_ADDRESS + i + 4));
+ reg_data = _nbu2ss_readl(IO_ADDRESS(USB_BASE_ADDRESS + i + 4));
dev_dbg(&udc->dev, " %08x", (int)reg_data);
- reg_data = _nbu2ss_readl(
- (u32 *)IO_ADDRESS(USB_BASE_ADDRESS + i + 8));
+ reg_data = _nbu2ss_readl(IO_ADDRESS(USB_BASE_ADDRESS + i + 8));
dev_dbg(&udc->dev, " %08x", (int)reg_data);
- reg_data = _nbu2ss_readl(
- (u32 *)IO_ADDRESS(USB_BASE_ADDRESS + i + 12));
+ reg_data = _nbu2ss_readl(IO_ADDRESS(USB_BASE_ADDRESS + i + 12));
dev_dbg(&udc->dev, " %08x\n", (int)reg_data);
}
--
2.7.4
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 1/2] staging: emxx_udc: Align parameter with parenthesis
Align parameters with parenthesis and removed
open parenthesis at the end of line.
Signed-off-by: Cristian Sicilia
---
drivers/staging/emxx_udc/emxx_udc.c | 270 ++--
1 file changed, 107 insertions(+), 163 deletions(-)
diff --git a/drivers/staging/emxx_udc/emxx_udc.c
b/drivers/staging/emxx_udc/emxx_udc.c
index bf7c5da..ebc622f 100644
--- a/drivers/staging/emxx_udc/emxx_udc.c
+++ b/drivers/staging/emxx_udc/emxx_udc.c
@@ -163,11 +163,9 @@ static void _nbu2ss_ep0_complete(struct usb_ep *_ep,
struct usb_request *_req)
/*-*/
/* Initialization usb_request */
-static void _nbu2ss_create_ep0_packet(
- struct nbu2ss_udc *udc,
- void *p_buf,
- unsigned length
-)
+static void _nbu2ss_create_ep0_packet(struct nbu2ss_udc *udc,
+ void *p_buf,
+ unsigned int length)
{
udc->ep0_req.req.buf= p_buf;
udc->ep0_req.req.length = length;
@@ -419,12 +417,8 @@ static void _nbu2ss_ep_dma_abort(struct nbu2ss_udc *udc,
struct nbu2ss_ep *ep)
/*-*/
/* Start IN Transfer */
-static void _nbu2ss_ep_in_end(
- struct nbu2ss_udc *udc,
- u32 epnum,
- u32 data32,
- u32 length
-)
+static void _nbu2ss_ep_in_end(struct nbu2ss_udc *udc, u32 epnum, u32 data32,
+ u32 length)
{
u32 data;
u32 num;
@@ -462,45 +456,41 @@ static void _nbu2ss_ep_in_end(
#ifdef USE_DMA
/*-*/
-static void _nbu2ss_dma_map_single(
- struct nbu2ss_udc *udc,
- struct nbu2ss_ep *ep,
- struct nbu2ss_req *req,
- u8 direct
-)
+static void _nbu2ss_dma_map_single(struct nbu2ss_udc *udc,
+ struct nbu2ss_ep *ep,
+ struct nbu2ss_req *req,
+ u8 direct)
{
if (req->req.dma == DMA_ADDR_INVALID) {
if (req->unaligned) {
req->req.dma = ep->phys_buf;
} else {
- req->req.dma = dma_map_single(
- udc->gadget.dev.parent,
- req->req.buf,
- req->req.length,
- (direct == USB_DIR_IN)
- ? DMA_TO_DEVICE : DMA_FROM_DEVICE);
+ req->req.dma = dma_map_single(udc->gadget.dev.parent,
+ req->req.buf,
+ req->req.length,
+ (direct == USB_DIR_IN) ?
+ DMA_TO_DEVICE :
+ DMA_FROM_DEVICE);
}
req->mapped = 1;
} else {
if (!req->unaligned)
- dma_sync_single_for_device(
- udc->gadget.dev.parent,
- req->req.dma,
- req->req.length,
- (direct == USB_DIR_IN)
- ? DMA_TO_DEVICE : DMA_FROM_DEVICE);
+ dma_sync_single_for_device(udc->gadget.dev.parent,
+ req->req.dma,
+ req->req.length,
+ (direct == USB_DIR_IN) ?
+ DMA_TO_DEVICE :
+ DMA_FROM_DEVICE);
req->mapped = 0;
}
}
/*-*/
-static void _nbu2ss_dma_unmap_single(
- struct nbu2ss_udc *udc,
- struct nbu2ss_ep *ep,
- struct nbu2ss_req *req,
- u8 direct
-)
+static void _nbu2ss_dma_unmap_single(struct nbu2ss_udc *udc,
+struct nbu2ss_ep *ep,
+struct nbu2ss_req *req,
+u8 direct)
{
u8 data[4];
u8 *p;
@@ -671,10 +661,8 @@ static int EP0_receive_NULL(struct nbu2ss_udc *udc, bool
pid_flag)
}
/*-*/
-static int _nbu2ss_ep0_in_transfer(
- struct nbu2ss_udc *udc,
- struct nbu2ss_req *req
-)
+static int _nbu2ss_ep0_in_transfer(struct nbu2ss_udc *udc,
+ struct nbu2ss_req *req)
{
u8 *p_buffer; /* IN Data Buffer */
u32
[PATCH 0/2] Parenthesis fix
In the first patch there is a fix of some parenthesis that terminate on end of line and some parameters that are not aligned with previous parenthesis. The second patch is a proposal that avoid the long line refactoring code removing cast pointer. Cristian Sicilia (2): staging: emxx_udc: Align parameter with parenthesis staging: emxx_udc: Remove cast and move all in one line. drivers/staging/emxx_udc/emxx_udc.c | 282 ++-- 1 file changed, 111 insertions(+), 171 deletions(-) -- 2.7.4 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] scsi: vmw_pscsi: Rearrange code to avoid multiple calls to free_irq during unload
On Tue, Nov 27, 2018 at 02:28:53PM -0500, Cathy Avery wrote: > Currently pvscsi_remove calls free_irq more than once as > pvscsi_release_resources and __pvscsi_shutdown both call > pvscsi_shutdown_intr. This results in a 'Trying to free > already-free IRQ' warning and stack trace. To solve the problem > pvscsi_shutdown_intr has been moved out of pvscsi_release_resources. > > Signed-off-by: Cathy Avery > Reviewed-by: Ewan D. Milne That bug has been there since the driver was introduced in 2009. It's so amazing that no one has fixed it until now... Reviewed-by: Dan Carpenter regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: android: ion: Add chunk heap initialization
On 11/27/18 9:20 PM, Laura Abbott wrote:
> On 11/26/18 10:43 AM, Alexey Skidanov wrote:
>>
>>
>> On 11/26/18 6:39 PM, Laura Abbott wrote:
>>> On 11/25/18 2:02 PM, Alexey Skidanov wrote:
On 11/25/18 11:40 PM, Laura Abbott wrote:
> On 11/25/18 1:22 PM, Alexey Skidanov wrote:
>>
>>
>> On 11/25/18 10:51 PM, Laura Abbott wrote:
>>> On 11/11/18 11:29 AM, Alexey Skidanov wrote:
Create chunk heap of specified size and base address by adding
"ion_chunk_heap=size@start" kernel boot parameter.
Signed-off-by: Alexey Skidanov
---
drivers/staging/android/ion/ion_chunk_heap.c | 40
1 file changed, 40 insertions(+)
diff --git a/drivers/staging/android/ion/ion_chunk_heap.c
b/drivers/staging/android/ion/ion_chunk_heap.c
index 159d72f..67573aa4 100644
--- a/drivers/staging/android/ion/ion_chunk_heap.c
+++ b/drivers/staging/android/ion/ion_chunk_heap.c
@@ -135,6 +135,7 @@ struct ion_heap *ion_chunk_heap_create(struct
ion_platform_heap *heap_data)
}
chunk_heap->base = heap_data->base;
chunk_heap->size = heap_data->size;
+ chunk_heap->heap.name = heap_data->name;
chunk_heap->allocated = 0;
gen_pool_add(chunk_heap->pool, chunk_heap->base,
heap_data->size, -1);
@@ -151,3 +152,42 @@ struct ion_heap *ion_chunk_heap_create(struct
ion_platform_heap *heap_data)
return ERR_PTR(ret);
}
+static u64 base;
+static u64 size;
+
+static int __init setup_heap(char *param)
+{
+ char *p, *pp;
+
+ size = memparse(param, &p);
+ if (param == p)
+ return -EINVAL;
+
+ if (*p == '@')
+ base = memparse(p + 1, &pp);
+ else
+ return -EINVAL;
+
+ if (p == pp)
+ return -EINVAL;
+
+ return 0;
+}
+
+__setup("ion_chunk_heap=", setup_heap);
+
+static int ion_add_chunk_heap(void)
+{
+ struct ion_heap *heap;
+ struct ion_platform_heap plat_heap = {.base = base,
+ .size = size,
+ .name = "chunk_heap",
+ .priv = (void *)PAGE_SIZE};
+ heap = ion_chunk_heap_create(&plat_heap);
+ if (heap)
+ ion_device_add_heap(heap);
+
+ return 0;
+}
+device_initcall(ion_add_chunk_heap);
+
>>>
>>> This solves a problem but not enough of the problem.
>>>
>>> We need to be able to support more than one chunk/carveout
>>> heap.
>> This is easy to support.
>> This also assumes that the memory has already been
>>> reserved/placed and that you know the base and size to
>>> pass on the command line. Part of the issue with the carveout
>>> heaps is that we need a way to tell the kernel to reserve
>>> the memory early enough and then get that information to
>>> Ion. Hard coding memory locations tends to be buggy from
>>> my past experience with Ion.
>> memmap= kernel option marks the memory region(s) as reserved (Zone
>> Allocator doesn't use this memory region(s)). So the heap(s) may
>> manage
>> this memory region(s).
>
> memmap= is x86 only. I really don't like using the command line for
> specifying the base/size as it seems likely to conflict with platforms
> that rely on devicetree for reserving memory regions.
>
> Thanks,
> Laura
>
I see ... So probably the better way is the one similar to this
https://elixir.bootlin.com/linux/latest/source/kernel/dma/contiguous.c#L245
?
>>>
>>> Correct. For platforms that need devicetree, we need a way to specify
>>> that a region should become an Ion heap. I went through a similar
>>> exercise with CMA heaps before I kind of gave up on figuring out a
>>> binding and just had Ion enumerate all CMA heaps. We do still need
>>> a solution to work with non-DT platforms as well so whatever we
>>> come up with needs to plausibly work for both cases. Your solution
>>> would cover the non-DT case but I'd really like to make sure we
>>> at least have a path forward for the devicetree case as well.
>>
>> I would say that we have the following steps to consider:
>>
>> 1. Memory reservation. The suggested solution doesn't care how it's done.
>>
>> 2. Per-heap information passing to the Kernel. It's different for DT and
>> non-DT cases.
>>
>> 3. Heap objects instantiation. The DT and non-DT cases have different
>> ways/formats to pass this per-heap informatio
[PATCH] scsi: vmw_pscsi: Rearrange code to avoid multiple calls to free_irq during unload
Currently pvscsi_remove calls free_irq more than once as
pvscsi_release_resources and __pvscsi_shutdown both call
pvscsi_shutdown_intr. This results in a 'Trying to free
already-free IRQ' warning and stack trace. To solve the problem
pvscsi_shutdown_intr has been moved out of pvscsi_release_resources.
Signed-off-by: Cathy Avery
Reviewed-by: Ewan D. Milne
---
drivers/scsi/vmw_pvscsi.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/scsi/vmw_pvscsi.c b/drivers/scsi/vmw_pvscsi.c
index 6e49102..0d6b2a8 100644
--- a/drivers/scsi/vmw_pvscsi.c
+++ b/drivers/scsi/vmw_pvscsi.c
@@ -1202,8 +1202,6 @@ static void pvscsi_shutdown_intr(struct pvscsi_adapter
*adapter)
static void pvscsi_release_resources(struct pvscsi_adapter *adapter)
{
- pvscsi_shutdown_intr(adapter);
-
if (adapter->workqueue)
destroy_workqueue(adapter->workqueue);
@@ -1534,6 +1532,7 @@ static int pvscsi_probe(struct pci_dev *pdev, const
struct pci_device_id *id)
out_reset_adapter:
ll_adapter_reset(adapter);
out_release_resources:
+ pvscsi_shutdown_intr(adapter);
pvscsi_release_resources(adapter);
scsi_host_put(host);
out_disable_device:
@@ -1542,6 +1541,7 @@ static int pvscsi_probe(struct pci_dev *pdev, const
struct pci_device_id *id)
return error;
out_release_resources_and_disable:
+ pvscsi_shutdown_intr(adapter);
pvscsi_release_resources(adapter);
goto out_disable_device;
}
--
2.5.5
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: android: ion: Add chunk heap initialization
On 11/26/18 10:43 AM, Alexey Skidanov wrote:
On 11/26/18 6:39 PM, Laura Abbott wrote:
On 11/25/18 2:02 PM, Alexey Skidanov wrote:
On 11/25/18 11:40 PM, Laura Abbott wrote:
On 11/25/18 1:22 PM, Alexey Skidanov wrote:
On 11/25/18 10:51 PM, Laura Abbott wrote:
On 11/11/18 11:29 AM, Alexey Skidanov wrote:
Create chunk heap of specified size and base address by adding
"ion_chunk_heap=size@start" kernel boot parameter.
Signed-off-by: Alexey Skidanov
---
drivers/staging/android/ion/ion_chunk_heap.c | 40
1 file changed, 40 insertions(+)
diff --git a/drivers/staging/android/ion/ion_chunk_heap.c
b/drivers/staging/android/ion/ion_chunk_heap.c
index 159d72f..67573aa4 100644
--- a/drivers/staging/android/ion/ion_chunk_heap.c
+++ b/drivers/staging/android/ion/ion_chunk_heap.c
@@ -135,6 +135,7 @@ struct ion_heap *ion_chunk_heap_create(struct
ion_platform_heap *heap_data)
}
chunk_heap->base = heap_data->base;
chunk_heap->size = heap_data->size;
+ chunk_heap->heap.name = heap_data->name;
chunk_heap->allocated = 0;
gen_pool_add(chunk_heap->pool, chunk_heap->base,
heap_data->size, -1);
@@ -151,3 +152,42 @@ struct ion_heap *ion_chunk_heap_create(struct
ion_platform_heap *heap_data)
return ERR_PTR(ret);
}
+static u64 base;
+static u64 size;
+
+static int __init setup_heap(char *param)
+{
+ char *p, *pp;
+
+ size = memparse(param, &p);
+ if (param == p)
+ return -EINVAL;
+
+ if (*p == '@')
+ base = memparse(p + 1, &pp);
+ else
+ return -EINVAL;
+
+ if (p == pp)
+ return -EINVAL;
+
+ return 0;
+}
+
+__setup("ion_chunk_heap=", setup_heap);
+
+static int ion_add_chunk_heap(void)
+{
+ struct ion_heap *heap;
+ struct ion_platform_heap plat_heap = {.base = base,
+ .size = size,
+ .name = "chunk_heap",
+ .priv = (void *)PAGE_SIZE};
+ heap = ion_chunk_heap_create(&plat_heap);
+ if (heap)
+ ion_device_add_heap(heap);
+
+ return 0;
+}
+device_initcall(ion_add_chunk_heap);
+
This solves a problem but not enough of the problem.
We need to be able to support more than one chunk/carveout
heap.
This is easy to support.
This also assumes that the memory has already been
reserved/placed and that you know the base and size to
pass on the command line. Part of the issue with the carveout
heaps is that we need a way to tell the kernel to reserve
the memory early enough and then get that information to
Ion. Hard coding memory locations tends to be buggy from
my past experience with Ion.
memmap= kernel option marks the memory region(s) as reserved (Zone
Allocator doesn't use this memory region(s)). So the heap(s) may manage
this memory region(s).
memmap= is x86 only. I really don't like using the command line for
specifying the base/size as it seems likely to conflict with platforms
that rely on devicetree for reserving memory regions.
Thanks,
Laura
I see ... So probably the better way is the one similar to this
https://elixir.bootlin.com/linux/latest/source/kernel/dma/contiguous.c#L245
?
Correct. For platforms that need devicetree, we need a way to specify
that a region should become an Ion heap. I went through a similar
exercise with CMA heaps before I kind of gave up on figuring out a
binding and just had Ion enumerate all CMA heaps. We do still need
a solution to work with non-DT platforms as well so whatever we
come up with needs to plausibly work for both cases. Your solution
would cover the non-DT case but I'd really like to make sure we
at least have a path forward for the devicetree case as well.
I would say that we have the following steps to consider:
1. Memory reservation. The suggested solution doesn't care how it's done.
2. Per-heap information passing to the Kernel. It's different for DT and
non-DT cases.
3. Heap objects instantiation. The DT and non-DT cases have different
ways/formats to pass this per-heap information. But once the parsing is
done, the rest of the code is common.
I think it clearly defines the steps covering both cases. What do you think?
Yes, that sounds about right.
Thanks,
Alexey
Thanks,
Laura
Thanks,
Alexey
If you'd like to see about coming up with a complete solution,
feel free to resubmit but I'm still strongly considering
removing these heaps.
I will add the multiple heaps support and resubmit the patch
Thanks,
Laura
Thanks,
Alexey
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH V2] staging: iio: adc: ad7280a: check for devm_kasprint() failure
devm_kasprintf() may return NULL on failure of internal allocation thus
the assignments to attr.name are not safe if not checked. On error
ad7280_attr_init() returns a negative return so -ENOMEM should be
OK here (passed on as return value of the probe function). To make the
error case more readable a temporary iio_attr is introduced and the code
refactored.
Signed-off-by: Nicholas Mc Guire
Fixes: 2051f25d2a26 ("iio: adc: New driver for AD7280A Lithium Ion Battery
Monitoring System2")
---
V2: use a tmp pointer to iio_attr to make the error check more readable
as proposed by Dan Carpenter
Problem located with an experimental coccinelle script
Patch was compile tested with: x86_64_defconfig + STAGING=y
SPI=y, IIO=y, AD7280=m
Patch is against 4.20-rc4 (localversion-next is next-20181127)
drivers/staging/iio/adc/ad7280a.c | 43 +++
1 file changed, 21 insertions(+), 22 deletions(-)
diff --git a/drivers/staging/iio/adc/ad7280a.c
b/drivers/staging/iio/adc/ad7280a.c
index 0bb9ab1..7a0ba26 100644
--- a/drivers/staging/iio/adc/ad7280a.c
+++ b/drivers/staging/iio/adc/ad7280a.c
@@ -561,6 +561,7 @@ static int ad7280_attr_init(struct ad7280_state *st)
{
int dev, ch, cnt;
unsigned int index;
+ struct iio_dev_attr *iio_attr;
st->iio_attr = devm_kcalloc(&st->spi->dev, 2, sizeof(*st->iio_attr) *
(st->slave_num + 1) * AD7280A_CELLS_PER_DEV,
@@ -571,37 +572,35 @@ static int ad7280_attr_init(struct ad7280_state *st)
for (dev = 0, cnt = 0; dev <= st->slave_num; dev++)
for (ch = AD7280A_CELL_VOLTAGE_1; ch <= AD7280A_CELL_VOLTAGE_6;
ch++, cnt++) {
+ iio_attr = &st->iio_attr[cnt];
index = dev * AD7280A_CELLS_PER_DEV + ch;
- st->iio_attr[cnt].address =
- ad7280a_devaddr(dev) << 8 | ch;
- st->iio_attr[cnt].dev_attr.attr.mode =
- 0644;
- st->iio_attr[cnt].dev_attr.show =
- ad7280_show_balance_sw;
- st->iio_attr[cnt].dev_attr.store =
- ad7280_store_balance_sw;
- st->iio_attr[cnt].dev_attr.attr.name =
+ iio_attr->address = ad7280a_devaddr(dev) << 8 | ch;
+ iio_attr->dev_attr.attr.mode = 0644;
+ iio_attr->dev_attr.show = ad7280_show_balance_sw;
+ iio_attr->dev_attr.store = ad7280_store_balance_sw;
+ iio_attr->dev_attr.attr.name =
devm_kasprintf(&st->spi->dev, GFP_KERNEL,
"in%d-in%d_balance_switch_en",
index, index + 1);
- ad7280_attributes[cnt] =
- &st->iio_attr[cnt].dev_attr.attr;
+ if (!iio_attr->dev_attr.attr.name)
+ return -ENOMEM;
+
+ ad7280_attributes[cnt] = &iio_attr->dev_attr.attr;
cnt++;
- st->iio_attr[cnt].address =
- ad7280a_devaddr(dev) << 8 |
+ iio_attr = &st->iio_attr[cnt];
+ iio_attr->address = ad7280a_devaddr(dev) << 8 |
(AD7280A_CB1_TIMER + ch);
- st->iio_attr[cnt].dev_attr.attr.mode =
- 0644;
- st->iio_attr[cnt].dev_attr.show =
- ad7280_show_balance_timer;
- st->iio_attr[cnt].dev_attr.store =
- ad7280_store_balance_timer;
- st->iio_attr[cnt].dev_attr.attr.name =
+ iio_attr->dev_attr.attr.mode = 0644;
+ iio_attr->dev_attr.show = ad7280_show_balance_timer;
+ iio_attr->dev_attr.store = ad7280_store_balance_timer;
+ iio_attr->dev_attr.attr.name =
devm_kasprintf(&st->spi->dev, GFP_KERNEL,
"in%d-in%d_balance_timer",
index, index + 1);
- ad7280_attributes[cnt] =
- &st->iio_attr[cnt].dev_attr.attr;
+ if (!iio_attr->dev_attr.attr.name)
+ return -ENOMEM;
+
+ ad7280_attributes[cnt] = &iio_attr->dev_attr.attr;
}
ad7280_attributes[cnt] = NULL;
--
2.1.4
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH RFC] mm/memory_hotplug: Introduce memory block types
On 27.11.18 17:32, Michal Suchánek wrote: > On Mon, 26 Nov 2018 16:59:14 +0100 > David Hildenbrand wrote: > >> On 26.11.18 15:20, Michal Suchánek wrote: >>> On Mon, 26 Nov 2018 14:33:29 +0100 >>> David Hildenbrand wrote: >>> On 26.11.18 13:30, David Hildenbrand wrote: > On 23.11.18 19:06, Michal Suchánek wrote: >>> >> >> If we are going to fake the driver information we may as well add the >> type attribute and be done with it. >> >> I think the problem with the patch was more with the semantic than the >> attribute itself. >> >> What is normal, paravirtualized, and standby memory? >> >> I can understand DIMM device, baloon device, or whatever mechanism for >> adding memory you might have. >> >> I can understand "memory designated as standby by the cluster >> administrator". >> >> However, DIMM vs baloon is orthogonal to standby and should not be >> conflated into one property. >> >> paravirtualized means nothing at all in relationship to memory type and >> the desired online policy to me. > > Right, so with whatever we come up, it should allow to make a decision > in user space about > - if memory is to be onlined automatically And I will think about if we really should model standby memory. Maybe it is really better to have in user space something like (as Dan noted) >>> >>> If it is possible to designate the memory as standby or online in the >>> s390 admin interface and the kernel does have access to this >>> information it makes sense to forward it to userspace (as separate >>> s390-specific property). If not then you need to make some kind of >>> assumption like below and the user can tune the script according to >>> their usecase. >> >> Also true, standby memory really represents a distinct type of memory >> block (memory seems to be there but really isn't). Right now I am >> thinking about something like this (tried to formulate it on a very >> generic level because we can't predict which mechanism might want to >> make use of these types in the future). >> >> >> /* >> * Memory block types allow user space to formulate rules if and how to >> * online memory blocks. The types are exposed to user space as text >> * strings in sysfs. While the typical online strategies are described >> * along with the types, there are use cases where that can differ (e.g. >> * use MOVABLE zone for more reliable huge page usage, use NORMAL zone >> * due to zone imbalance or because memory unplug is not intended). >> * >> * MEMORY_BLOCK_NONE: >> * No memory block is to be created (e.g. device memory). Used internally >> * only. >> * >> * MEMORY_BLOCK_REMOVABLE: >> * This memory block type should be treated as if it can be >> * removed/unplugged from the system again. E.g. there is a hardware >> * interface to unplug such memory. This memory block type is usually >> * onlined to the MOVABLE zone, to e.g. make offlining of it more >> * reliable. Examples include ACPI and PPC DIMMs. >> * >> * MEMORY_BLOCK_UNREMOVABLE: >> * This memory block type should be treated as if it can not be >> * removed/unplugged again. E.g. there is no hardware interface to >> * unplug such memory. This memory block type is usually onlined to >> * the NORMAL zone, as offlining is not beneficial. Examples include boot >> * memory on most architectures and memory added via balloon devices. > > AFAIK baloon device can be inflated as well so this does not really > describe how this memory type works in any meaningful way. Also it > should not be possible to see this kind of memory from userspace. The > baloon driver just takes existing memory that is properly backed, > allocates it for itself, and allows the hypervisor to use it. Thus it > creates the equivalent to s390 standby memory which is not backed in > the VM. When memory is reclaimed from hypervisor the baloon driver > frees it making it available to the VM kernel again. However, the whole > time the memory appears present in the machine and no hotplug events > should be visible unless the docs I am looking at are really outdated. It's all not optimal yet. Don't confuse what I describe here with inflated/deflated memory. XEN and Hyper-V add *new* memory to the system using add_memory(). New memory blocks. This memory will never be removed using the typical "offline + remove_memory()" approach. It will be removed using ballooning (if at all) and only in pieces. So it will usually be onlined to the NORMAL zone. (but userspace can later on implement whatever rule it wants) I am not talking about any kind of inflation/deflation. I am talking about memory blocks added to the system via add_memory(). Inflation/deflation does not belong into the memory block interface. > >> * >> * MEMORY_BLOCK_STANDBY: >> * The memory block type should be treated as if it can be >> * removed/unplugged again, however the ac
Re: [PATCH] staging: comedi: fix spelling mistake "desination" -> "destination"
On 27/11/2018 14:23, Colin King wrote: From: Colin Ian King There is a spelling mistake in message text in the call to unittest, fix this. Signed-off-by: Colin Ian King --- drivers/staging/comedi/drivers/tests/ni_routes_test.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/staging/comedi/drivers/tests/ni_routes_test.c b/drivers/staging/comedi/drivers/tests/ni_routes_test.c index a1eda035f270..c6dc18f346e8 100644 --- a/drivers/staging/comedi/drivers/tests/ni_routes_test.c +++ b/drivers/staging/comedi/drivers/tests/ni_routes_test.c @@ -372,7 +372,7 @@ void test_ni_lookup_route_register(void) unittest(ni_lookup_route_register(O(8), O(9), T) == 8, "validate last destination\n"); unittest(ni_lookup_route_register(O(10), O(9), T) == -EINVAL, -"lookup invalid desination\n"); +"lookup invalid destination\n"); unittest(ni_lookup_route_register(rgout0_src0, TRIGGER_LINE(0), T) == -EINVAL, Thanks for spotting that! Looks good. Reviewed-by: Ian Abbott -- -=( Ian Abbott || Web: www.mev.co.uk )=- -=( MEV Ltd. is a company registered in England & Wales. )=- -=( Registered number: 02862268. Registered address:)=- -=( 15 West Park Road, Bramhall, STOCKPORT, SK7 3JZ, UK. )=- ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
RE: [PATCH v2] makedumpfile: exclude pages that are logically offline
> Linux marks pages that are logically offline via a page flag (map count).
> Such pages e.g. include pages infated as part of a balloon driver or
> pages that were not actually onlined when onlining the whole section.
>
> While the hypervisor usually allows to read such inflated memory, we
> basically read and dump data that is completely irrelevant. Also, this
> might result in quite some overhead in the hypervisor. In addition,
> we saw some problems under Hyper-V, whereby we can crash the kernel by
> dumping, when reading memory of a partially onlined memory segment
> (for memory added by the Hyper-V balloon driver).
>
> Therefore, don't read and dump pages that are marked as being logically
> offline.
>
> Signed-off-by: David Hildenbrand
Thanks for the v2 update.
I'm going to merge this patch after the kernel patches are merged
and it tests fine with the kernel.
Kazu
> ---
>
> v1 -> v2:
> - Fix PAGE_BUDDY_MAPCOUNT_VALUE vs. PAGE_OFFLINE_MAPCOUNT_VALUE
>
> makedumpfile.c | 34 ++
> makedumpfile.h | 1 +
> 2 files changed, 31 insertions(+), 4 deletions(-)
>
> diff --git a/makedumpfile.c b/makedumpfile.c
> index 8923538..a5f2ea9 100644
> --- a/makedumpfile.c
> +++ b/makedumpfile.c
> @@ -88,6 +88,7 @@ mdf_pfn_t pfn_cache_private;
> mdf_pfn_t pfn_user;
> mdf_pfn_t pfn_free;
> mdf_pfn_t pfn_hwpoison;
> +mdf_pfn_t pfn_offline;
>
> mdf_pfn_t num_dumped;
>
> @@ -249,6 +250,21 @@ isHugetlb(unsigned long dtor)
> && (SYMBOL(free_huge_page) == dtor));
> }
>
> +static int
> +isOffline(unsigned long flags, unsigned int _mapcount)
> +{
> + if (NUMBER(PAGE_OFFLINE_MAPCOUNT_VALUE) == NOT_FOUND_NUMBER)
> + return FALSE;
> +
> + if (flags & (1UL << NUMBER(PG_slab)))
> + return FALSE;
> +
> + if (_mapcount == (int)NUMBER(PAGE_OFFLINE_MAPCOUNT_VALUE))
> + return TRUE;
> +
> + return FALSE;
> +}
> +
> static int
> is_cache_page(unsigned long flags)
> {
> @@ -2287,6 +2303,8 @@ write_vmcoreinfo_data(void)
> WRITE_NUMBER("PG_hwpoison", PG_hwpoison);
>
> WRITE_NUMBER("PAGE_BUDDY_MAPCOUNT_VALUE", PAGE_BUDDY_MAPCOUNT_VALUE);
> + WRITE_NUMBER("PAGE_OFFLINE_MAPCOUNT_VALUE",
> + PAGE_OFFLINE_MAPCOUNT_VALUE);
> WRITE_NUMBER("phys_base", phys_base);
>
> WRITE_NUMBER("HUGETLB_PAGE_DTOR", HUGETLB_PAGE_DTOR);
> @@ -2687,6 +2705,7 @@ read_vmcoreinfo(void)
> READ_SRCFILE("pud_t", pud_t);
>
> READ_NUMBER("PAGE_BUDDY_MAPCOUNT_VALUE", PAGE_BUDDY_MAPCOUNT_VALUE);
> + READ_NUMBER("PAGE_OFFLINE_MAPCOUNT_VALUE", PAGE_OFFLINE_MAPCOUNT_VALUE);
> READ_NUMBER("phys_base", phys_base);
> #ifdef __aarch64__
> READ_NUMBER("VA_BITS", VA_BITS);
> @@ -6041,6 +6060,12 @@ __exclude_unnecessary_pages(unsigned long mem_map,
> else if (isHWPOISON(flags)) {
> pfn_counter = &pfn_hwpoison;
> }
> + /*
> + * Exclude pages that are logically offline.
> + */
> + else if (isOffline(flags, _mapcount)) {
> + pfn_counter = &pfn_offline;
> + }
> /*
>* Unexcludable page
>*/
> @@ -7522,7 +7547,7 @@ write_elf_pages_cyclic(struct cache_data *cd_header,
> struct cache_data *cd_page)
>*/
> if (info->flag_cyclic) {
> pfn_zero = pfn_cache = pfn_cache_private = 0;
> - pfn_user = pfn_free = pfn_hwpoison = 0;
> + pfn_user = pfn_free = pfn_hwpoison = pfn_offline = 0;
> pfn_memhole = info->max_mapnr;
> }
>
> @@ -8804,7 +8829,7 @@ write_kdump_pages_and_bitmap_cyclic(struct cache_data
> *cd_header, struct cache_d
>* Reset counter for debug message.
>*/
> pfn_zero = pfn_cache = pfn_cache_private = 0;
> - pfn_user = pfn_free = pfn_hwpoison = 0;
> + pfn_user = pfn_free = pfn_hwpoison = pfn_offline = 0;
> pfn_memhole = info->max_mapnr;
>
> /*
> @@ -9749,7 +9774,7 @@ print_report(void)
> pfn_original = info->max_mapnr - pfn_memhole;
>
> pfn_excluded = pfn_zero + pfn_cache + pfn_cache_private
> - + pfn_user + pfn_free + pfn_hwpoison;
> + + pfn_user + pfn_free + pfn_hwpoison + pfn_offline;
> shrinking = (pfn_original - pfn_excluded) * 100;
> shrinking = shrinking / pfn_original;
>
> @@ -9763,6 +9788,7 @@ print_report(void)
> REPORT_MSG("User process data pages : 0x%016llx\n", pfn_user);
> REPORT_MSG("Free pages : 0x%016llx\n", pfn_free);
> REPORT_MSG("Hwpoison pages : 0x%016llx\n", pfn_hwpoison);
> + REPORT_MSG("Offline pages : 0x%016llx\n", pfn_offline);
> REPORT_MSG(" Remaining pages : 0x%016llx\n",
> pfn_original - pfn_excluded);
> REPORT_MSG(" (The number of pages is reduced to %lld%%.)\n",
> @@ -9790,7 +9816,7
Re: [PATCH RFC] mm/memory_hotplug: Introduce memory block types
On Mon, 26 Nov 2018 16:59:14 +0100 David Hildenbrand wrote: > On 26.11.18 15:20, Michal Suchánek wrote: > > On Mon, 26 Nov 2018 14:33:29 +0100 > > David Hildenbrand wrote: > > > >> On 26.11.18 13:30, David Hildenbrand wrote: > >>> On 23.11.18 19:06, Michal Suchánek wrote: > > > > If we are going to fake the driver information we may as well add the > type attribute and be done with it. > > I think the problem with the patch was more with the semantic than the > attribute itself. > > What is normal, paravirtualized, and standby memory? > > I can understand DIMM device, baloon device, or whatever mechanism for > adding memory you might have. > > I can understand "memory designated as standby by the cluster > administrator". > > However, DIMM vs baloon is orthogonal to standby and should not be > conflated into one property. > > paravirtualized means nothing at all in relationship to memory type and > the desired online policy to me. > >>> > >>> Right, so with whatever we come up, it should allow to make a decision > >>> in user space about > >>> - if memory is to be onlined automatically > >> > >> And I will think about if we really should model standby memory. Maybe > >> it is really better to have in user space something like (as Dan noted) > > > > If it is possible to designate the memory as standby or online in the > > s390 admin interface and the kernel does have access to this > > information it makes sense to forward it to userspace (as separate > > s390-specific property). If not then you need to make some kind of > > assumption like below and the user can tune the script according to > > their usecase. > > Also true, standby memory really represents a distinct type of memory > block (memory seems to be there but really isn't). Right now I am > thinking about something like this (tried to formulate it on a very > generic level because we can't predict which mechanism might want to > make use of these types in the future). > > > /* > * Memory block types allow user space to formulate rules if and how to > * online memory blocks. The types are exposed to user space as text > * strings in sysfs. While the typical online strategies are described > * along with the types, there are use cases where that can differ (e.g. > * use MOVABLE zone for more reliable huge page usage, use NORMAL zone > * due to zone imbalance or because memory unplug is not intended). > * > * MEMORY_BLOCK_NONE: > * No memory block is to be created (e.g. device memory). Used internally > * only. > * > * MEMORY_BLOCK_REMOVABLE: > * This memory block type should be treated as if it can be > * removed/unplugged from the system again. E.g. there is a hardware > * interface to unplug such memory. This memory block type is usually > * onlined to the MOVABLE zone, to e.g. make offlining of it more > * reliable. Examples include ACPI and PPC DIMMs. > * > * MEMORY_BLOCK_UNREMOVABLE: > * This memory block type should be treated as if it can not be > * removed/unplugged again. E.g. there is no hardware interface to > * unplug such memory. This memory block type is usually onlined to > * the NORMAL zone, as offlining is not beneficial. Examples include boot > * memory on most architectures and memory added via balloon devices. AFAIK baloon device can be inflated as well so this does not really describe how this memory type works in any meaningful way. Also it should not be possible to see this kind of memory from userspace. The baloon driver just takes existing memory that is properly backed, allocates it for itself, and allows the hypervisor to use it. Thus it creates the equivalent to s390 standby memory which is not backed in the VM. When memory is reclaimed from hypervisor the baloon driver frees it making it available to the VM kernel again. However, the whole time the memory appears present in the machine and no hotplug events should be visible unless the docs I am looking at are really outdated. > * > * MEMORY_BLOCK_STANDBY: > * The memory block type should be treated as if it can be > * removed/unplugged again, however the actual memory hot(un)plug is > * performed by onlining/offlining. In virtual environments, such memory > * is usually added during boot and never removed. Onlining memory will > * result in memory getting allocated to a VM. This memory type is usually > * not onlined automatically but explicitly by the administrator. One > * example is standby memory on s390x. Again, this does not meaningfully describe the memory type. There is no memory on standby. There is in fact no backing at all unless you online it. So this probably is some kind of shared memory. However, the (de)allocation is controlled differently compared to the baloon device. The concept is very similar, though. Thanks Michal ___ d
Re: [PATCH] staging: iio: ad7780: Add gain & filter gpio support
On Lu, 2018-11-26 at 17:24 -0200, Giuliano Belinassi wrote: Hi, please see bellow > Hi, thank you for the review > > > > > On Thu, 22 Nov 2018 11:01:00 + > > "Popa, Stefan Serban" wrote: > > > > > > I think that instead of setting the gain directly, we should use > > > the IIO_CHAN_INFO_SCALE attribute. At page 12 of the ad7780 datasheet > > > there > > > is a formula from which the output code can be calculated: > > > Code = 2^(N − 1) > > > × [(AIN × Gain /VREF) + 1]. So, by setting the scale from user space, > > > the > > > driver can calculate the correct gain by using the formula above. > > > Also, it > > > would be useful to introduce scale available. > > > Furthermore, there is a new > > > ad7124 adc driver which does this exact thing. Take a look here: http > > > s://gi > > > thub.com/analogdevicesinc/linux/blob/master/drivers/iio/adc/ad7124.c# > > > L337. > We have some questions about the code you provided to us: > 1-) What is exactly the inputs for the write_raw function? In your write_raw() function you need to add IIO_CHAN_INFO_SCALE case. Setting the scale from user space looks something like this: root:/sys/bus/iio/devices/iio:device0> echo 0.000298 > in_voltage_scale . Furthermore, in your write_raw() function, val=0 and val2=298. Knowing that full_scale_voltage = Vref / (gain * scale), we can calculate the gain = Vref / (full_scale_voltage * scale). We only support two gains (1 and 128), so we need to determine which one fits better with the desired scale. Finally, all we have left to do is to set the gain. > 2-) Could you give more details about the math around lines 346-348? > Is it correct to assume that the multiplication at line 346 won't > overflow? (vref is an uint) It is correct that Vref is in microvolts, so for example, Vref of 2.5V = 25uV. It won't overflow since we use the Vref as nominator, while full_scale_voltage and scale are the denominators. > > And regarding our code: > 1-) The val in our write_raw function should be, in case of GAIN, a > number that best approximate the actual gain value of 1 or 128? For > instance, if the user inputs 126, we should default to 128? We should not allow the the user to input the gain, he needs to input the scale (see the mail from Jonathan and the above explanation). However, if the calculated gain is one that is not supported, such as 126, we will set the closest matching value, in this case 128. > 2-) In the case of FILTER, is it the same? Is the user sending the > value in mHz (milihertz)? Yes, it is the same with the FILTER. You need to add a IIO_CHAN_INFO_SAMP_FREQ case in your write_raw() function. From user space, the input value should be in Hz, something like this: root:/sys/bus/iio/devices/iio:device0> echo 10 > in_voltage_sampling_frequency > > Thank you > ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] staging: rtlwifi: fix spelling mistake "disnabled" -> "disabled"
From: Colin Ian King
There is a spelling mistake in an ODM_RT_TRACE message, fix it.
Signed-off-by: Colin Ian King
---
drivers/staging/rtlwifi/phydm/phydm.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/staging/rtlwifi/phydm/phydm.c
b/drivers/staging/rtlwifi/phydm/phydm.c
index 27635feedba2..52b85b3ddb34 100644
--- a/drivers/staging/rtlwifi/phydm/phydm.c
+++ b/drivers/staging/rtlwifi/phydm/phydm.c
@@ -477,7 +477,7 @@ static void phydm_supportability_init(void *dm_void)
} else {
ODM_RT_TRACE(dm, ODM_COMP_INIT,
-"ODM adaptivity is set to disnabled!!!\n");
+"ODM adaptivity is set to disabled!!!\n");
/**/
}
--
2.19.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] staging: comedi: fix spelling mistake "desination" -> "destination"
From: Colin Ian King There is a spelling mistake in message text in the call to unittest, fix this. Signed-off-by: Colin Ian King --- drivers/staging/comedi/drivers/tests/ni_routes_test.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/staging/comedi/drivers/tests/ni_routes_test.c b/drivers/staging/comedi/drivers/tests/ni_routes_test.c index a1eda035f270..c6dc18f346e8 100644 --- a/drivers/staging/comedi/drivers/tests/ni_routes_test.c +++ b/drivers/staging/comedi/drivers/tests/ni_routes_test.c @@ -372,7 +372,7 @@ void test_ni_lookup_route_register(void) unittest(ni_lookup_route_register(O(8), O(9), T) == 8, "validate last destination\n"); unittest(ni_lookup_route_register(O(10), O(9), T) == -EINVAL, -"lookup invalid desination\n"); +"lookup invalid destination\n"); unittest(ni_lookup_route_register(rgout0_src0, TRIGGER_LINE(0), T) == -EINVAL, -- 2.19.1 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [RFC PATCH v2] android: ion: How to properly clean caches for uncached allocations
Hi Liam,
On Mon, Nov 26, 2018 at 08:59:44PM -0800, Liam Mark wrote:
> On Tue, 20 Nov 2018, Brian Starkey wrote:
>
> > Hi Liam,
> >
> > I'm missing a bit of context here, but I did read the v1 thread.
> > Please accept my apologies if I'm re-treading trodden ground.
> >
> > I do know we're chasing nebulous ion "problems" on our end, which
> > certainly seem to be related to what you're trying to fix here.
> >
> > On Thu, Nov 01, 2018 at 03:15:06PM -0700, Liam Mark wrote:
> > >Based on the suggestions from Laura I created a first draft for a change
> > >which will attempt to ensure that uncached mappings are only applied to
> > >ION memory who's cache lines have been cleaned.
> > >It does this by providing cached mappings (for uncached ION allocations)
> > >until the ION buffer is dma mapped and successfully cleaned, then it
> > drops
> > >the userspace mappings and when pages are accessed they are faulted back
> > >in and uncached mappings are created.
> >
> > If I understand right, there's no way to portably clean the cache of
> > the kernel mapping before we map the pages into userspace. Is that
> > right?
> >
>
> Yes, it isn't always possible to clean the caches for an uncached mapping
> because a device is required by the DMA APIs to do cache maintenance and
> there isn't necessarily a device available (dma_buf_attach may not yet
> have been called).
>
> > Alternatively, can we just make ion refuse to give userspace a
> > non-cached mapping for pages which are mapped in the kernel as cached?
>
> These pages will all be mapped as cached in the kernel for 64 bit (kernel
> logical addresses) so you would always be refusing to create a non-cached
> mapping.
And that might be the sane thing to do, no?
AFAIK there are still pages which aren't ever mapped as cached (e.g.
dma_declare_coherent_memory(), anything under /reserved-memory marked
as no-map). If those are exposed as an ion heap, then non-cached
mappings would be fine, and permitted.
>
> > Would userspace using the dma-buf sync ioctl around its accesses do
> > the "right thing" in that case?
> >
>
> I don't think so, the dma-buf sync ioctl require a device to peform cache
> maintenance, but as mentioned above a device may not be available.
>
If a device didn't attach yet, then no cache maintenance is
necessary. The only thing accessing the memory is the CPU, via a
cached mapping, which should work just fine. So far so good.
If there are already attachments, then ion_dma_buf_begin_cpu_access()
will sync for CPU access against all of the attached devices, and
again the CPU should see the right thing.
In the other direction, ion_dma_buf_end_cpu_access() will sync for
device access for all currently attached devices. If there's no
attached devices yet, then there's nothing to do until there is (only
thing accessing is CPU via a CPU-cached mapping).
When the first (or another) device attaches, then when it maps the
buffer, the map_dma_buf callback should do whatever sync-ing is needed
for that device.
I might be way off with my understanding of the various DMA APIs, but
this is how I think they're meant to work.
> > Given that as you pointed out, the kernel does still have a cached
> > mapping to these pages, trying to give the CPU a non-cached mapping of
> > those same pages while preserving consistency seems fraught. Wouldn't
> > it be better to make sure all CPU mappings are cached, and have CPU
> > clients use the dma_buf_{begin,end}_cpu_access() hooks to get
> > consistency where needed?
> >
>
> It is fraught, but unfortunately you can't rely on
> dma_buf_{begin,end}_cpu_access() to do cache maintenance as these calls
> require a device, and a device is not always available.
As above, if there's really no device, then no syncing is needed
because only the CPU is accessing the buffer, and only ever via cached
mappings.
>
> > >
> > >This change has the following potential disadvantages:
> > >- It assumes that userpace clients won't attempt to access the buffer
> > >while it is being mapped as we are removing the userpspace mappings at
> > >this point (though it is okay for them to have it mapped)
> > >- It assumes that kernel clients won't hold a kernel mapping to the
> > buffer
> > >(ie dma_buf_kmap) while it is being dma-mapped. What should we do if
> > there
> > >is a kernel mapping at the time of dma mapping, fail the mapping, warn?
> > >- There may be a performance penalty as a result of having to fault in
> > the
> > >pages after removing the userspace mappings.
> >
> > I wonder if the dma-buf sync ioctl might provide a way for userspace
> > to opt-in to when the zap/fault happens. Zap on (DMA_BUF_SYNC_WRITE |
> > DMA_BUF_SYNC_WRITE_END) and fault on (DMA_BUF_SYNC_READ |
> > DMA_BUF_SYNC_START)
> >
>
> Not sure I understand, can you elaborate.
> Are you also adding a requirment that ION pages can't be mmaped during a
> call to dma_buf_map_attachment?
I was only suggesting that zapping the mappings "at random
Re: [PATCH V5 00/10] x86/KVM/Hyper-v: Add HV ept tlb range flush hypercall support in KVM
On Tue, Nov 27, 2018 at 8:12 PM Dan Carpenter wrote: > > On Tue, Nov 27, 2018 at 07:59:22PM +0800, Tianyu Lan wrote: > > Gentile Ping... > > > > On Thu, Nov 8, 2018 at 10:43 PM wrote: > > > > > > From: Lan Tianyu > > > > > > Sorry. Some patches was blocked and I try to resend via another account. > > The patches were still blocked? They didn't show up on driver-devel. > > regards, > dan carpenter > Hi Dan: Thanks for reminder. I double check kvm maillist archive and this series is available. https://marc.info/?l=kvm&m=154168821118117&w=2 -- Best regards Tianyu Lan ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] Revert commit ef9209b642f "staging: rtl8723bs: Fix indenting errors and an off-by-one mistake in core/rtw_mlme_ext.c"
Thanks! Reviewed-by: Dan Carpenter regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH V5 00/10] x86/KVM/Hyper-v: Add HV ept tlb range flush hypercall support in KVM
On Tue, Nov 27, 2018 at 07:59:22PM +0800, Tianyu Lan wrote: > Gentile Ping... > > On Thu, Nov 8, 2018 at 10:43 PM wrote: > > > > From: Lan Tianyu > > > > Sorry. Some patches was blocked and I try to resend via another account. The patches were still blocked? They didn't show up on driver-devel. regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH V5 00/10] x86/KVM/Hyper-v: Add HV ept tlb range flush hypercall support in KVM
Gentile Ping... On Thu, Nov 8, 2018 at 10:43 PM wrote: > > From: Lan Tianyu > > Sorry. Some patches was blocked and I try to resend via another account. > > For nested memory virtualization, Hyper-v doesn't set write-protect > L1 hypervisor EPT page directory and page table node to track changes > while it relies on guest to tell it changes via HvFlushGuestAddressLlist > hypercall. HvFlushGuestAddressLlist hypercall provides a way to flush > EPT page table with ranges which are specified by L1 hypervisor. > > If L1 hypervisor uses INVEPT or HvFlushGuestAddressSpace hypercall to > flush EPT tlb, Hyper-V will invalidate associated EPT shadow page table > and sync L1's EPT table when next EPT page fault is triggered. > HvFlushGuestAddressLlist hypercall helps to avoid such redundant EPT > page fault and synchronization of shadow page table. > > This patchset is rebased on the Linux 4.20-rc1 and Patch "KVM/VMX: Check > ept_pointer before flushing ept tlb".(https://www.mail-archive.com/linux > -ker...@vger.kernel.org/msg1798827.html). > > Change since v4: >1) Split flush address and flush list patches. This patchset only > contains >flush address patches. Will post flush list patches later. >2) Expose function hyperv_fill_flush_guest_mapping_list() >out of hyperv file >3) Adjust parameter of hyperv_flush_guest_mapping_range() >4) Reorder patchset and move Hyper-V and VMX changes ahead. > > Change since v3: > 1) Remove code of updating "tlbs_dirty" in > kvm_flush_remote_tlbs_with_range() > 2) Remove directly tlb flush in the kvm_handle_hva_range() > 3) Move tlb flush in kvm_set_pte_rmapp() to > kvm_mmu_notifier_change_pte() > 4) Combine Vitaly's "don't pass EPT configuration info to > vmx_hv_remote_flush_tlb()" fix > > Change since v2: >1) Fix comment in the kvm_flush_remote_tlbs_with_range() >2) Move HV_MAX_FLUSH_PAGES and HV_MAX_FLUSH_REP_COUNT to > hyperv-tlfs.h. >3) Calculate HV_MAX_FLUSH_REP_COUNT in the macro definition >4) Use HV_MAX_FLUSH_REP_COUNT to define length of gpa_list in > struct hv_guest_mapping_flush_list. > > Change since v1: >1) Convert "end_gfn" of struct kvm_tlb_range to "pages" in order > to avoid confusion as to whether "end_gfn" is inclusive or exlusive. >2) Add hyperv tlb range struct and replace kvm tlb range struct > with new struct in order to avoid using kvm struct in the hyperv > code directly. > > > Lan Tianyu (10): > KVM: Add tlb_remote_flush_with_range callback in kvm_x86_ops > x86/hyper-v: Add HvFlushGuestAddressList hypercall support > x86/Hyper-v: Add trace in the > hyperv_nested_flush_guest_mapping_range() > KVM/VMX: Add hv tlb range flush support > KVM/MMU: Add tlb flush with range helper function > KVM: Replace old tlb flush function with new one to flush a specified > range. > KVM: Make kvm_set_spte_hva() return int > KVM/MMU: Move tlb flush in kvm_set_pte_rmapp() to > kvm_mmu_notifier_change_pte() > KVM/MMU: Flush tlb directly in the kvm_set_pte_rmapp() > KVM/MMU: Flush tlb directly in the kvm_zap_gfn_range() > > arch/arm/include/asm/kvm_host.h | 2 +- > arch/arm64/include/asm/kvm_host.h | 2 +- > arch/mips/include/asm/kvm_host.h| 2 +- > arch/mips/kvm/mmu.c | 3 +- > arch/powerpc/include/asm/kvm_host.h | 2 +- > arch/powerpc/kvm/book3s.c | 3 +- > arch/powerpc/kvm/e500_mmu_host.c| 3 +- > arch/x86/hyperv/nested.c| 80 +++ > arch/x86/include/asm/hyperv-tlfs.h | 32 + > arch/x86/include/asm/kvm_host.h | 9 +++- > arch/x86/include/asm/mshyperv.h | 15 ++ > arch/x86/include/asm/trace/hyperv.h | 14 ++ > arch/x86/kvm/mmu.c | 96 > + > arch/x86/kvm/paging_tmpl.h | 3 +- > arch/x86/kvm/vmx.c | 69 ++ > virt/kvm/arm/mmu.c | 6 ++- > virt/kvm/kvm_main.c | 5 +- > 17 files changed, 295 insertions(+), 51 deletions(-) > > -- > 2.14.4 > -- Best regards Tianyu Lan ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 3/4] Drivers: hv: vmbus: Add hooks for per-CPU IRQ
On Tue, Nov 27, 2018 at 07:20:56AM +0100, Greg KH wrote: > On Mon, Nov 26, 2018 at 08:56:50PM +, Michael Kelley wrote: > > From: Greg KH Monday, November 26, 2018 11:57 > > AM > > > > > > > You created "null" hooks that do nothing, for no one in this patch > > > > > series, why? > > > > > > > > > > > > > hv_enable_vmbus_irq() and hv_disable_vmbus_irq() have non-null > > > > implementations in the ARM64 code in patch 2 of this series. The > > > > implementations are in the new file arch/arm64/hyperv/mshyperv.c. > > > > Or am I misunderstanding your point? > > > > > > So you use a hook in an earlier patch and then add it in a later one? > > > > > > Shouldn't you do it the other way around? As it is, the earlier patch > > > should not work properly, right? > > > > The earlier patch implements the hook on the ARM64 side but it is > > unused -- it's not called. The later patch then calls it. Wouldn't the > > other way around be backwards? > > Ah, it wasn't obvious that the previous patch added it at all, why not > just make that addition part of this patch? > > > The general approach is for patches 1 and 2 of the series to provide > > all the new code under arch/arm64 to enable Hyper-V. But the code > > won't get called (or even built) with just these two patches because > > CONFIG_HYPERV can't be selected. Patch 3 is separate because it > > applies to architecture independent code and arch/x86 code -- I thought > > there might be value in keeping the ARM64 and x86 patches distinct. > > Patch 4 applies to architecture independent code, and enables the > > ARM64 code in patches 1 and 2 to be compiled and run when > > CONFIG_HYPERV is selected. > > > > If combining some of the patches in the series is a better approach, I'm > > good with that. > > Ok, that makes more sense, if it is easier to get the ARM people to > review this, that's fine. Doesn't seem like anyone did that yet :( It's on the list, but thanks for having a look as well! Will ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 00/16] staging: vchiq: dead code removal & misc fixes
On Mon, Nov 26, 2018 at 08:36:33PM +0100, Stefan Wahren wrote: > > Nicolas Saenz Julienne hat am 20. November 2018 um > > 15:53 geschrieben: > > > > > > Hi All, > > > > This series was written in parallel with reading and understanding the > > vchiq code. So excuse me for the lack of logic in the sequence of > > patches. > > > > The main focus was to delete as much code as possible, I've counted > > around 550 lines, which is not bad. Apart from that there are some > > patches enforcing proper kernel APIs usage. > > > > The only patch that really changes code is the > > vchiq_ioc_copy_element_data() rewrite. > > > > The last commit updates the TODO list with some of my observations, I > > realise some of the might be a little opinionated. If anything it's > > going to force a discussion on the topic, which is nice. > > > > It was developed on top of the latest linux-next, and was tested on a > > RPIv3B+ with audio, video and running vchiq_test. > > > > Regards, > > Nicolas > > > > RFC -> PATCH, as per Stefan's comments: > > - Remove semaphore initialization from remove_event_create() > > (commit 9) > > - Join all three semaphore to completion patches (commit 11) > > - Update probe/init commit message (commit 14) > > - Update TODO commit message and clean up (commit 16) > > - Fix spelling on some of the patches > > > > The whole series is > > Acked-by: Stefan Wahren > > Unfortunately patch 05 might not apply. I fixed it up. thanks, greg k-h ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] Revert commit ef9209b642f "staging: rtl8723bs: Fix indenting errors and an off-by-one mistake in core/rtw_mlme_ext.c"
From: Young Xiao
pstapriv->max_num_sta is always <= NUM_STA, since max_num_sta is either
set in _rtw_init_sta_priv() or rtw_set_beacon().
Signed-off-by: Young Xiao
---
drivers/staging/rtl8723bs/core/rtw_mlme_ext.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
b/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
index 69c7abc..8445d51 100644
--- a/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
+++ b/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
@@ -1565,7 +1565,7 @@ unsigned int OnAssocReq(struct adapter *padapter, union
recv_frame *precv_frame)
if (pstat->aid > 0) {
DBG_871X(" old AID %d\n", pstat->aid);
} else {
- for (pstat->aid = 1; pstat->aid < NUM_STA; pstat->aid++)
+ for (pstat->aid = 1; pstat->aid <= NUM_STA; pstat->aid++)
if (pstapriv->sta_aid[pstat->aid - 1] == NULL)
break;
--
2.7.4
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: rtl8188eu: Fix off-by-one in core/rtw_mlme_ext.c
Yes, you are right. I will send a patch to revert ef9209b642f. Young On 2018/11/27 16:49, Dan Carpenter wrote: > On Tue, Nov 27, 2018 at 08:41:53AM +, Yang Xiao wrote: >> Okay. I can send a patch to revert ef9209b642f. >> >> But, can you make sure that the condition "(pstapriv->sta_aid[pstat->aid >> - 1] == NULL)" can satisfies in the for loop? > ->max_num_sta is either set in _rtw_init_sta_priv() or rtw_set_beacon(). > You can see that it's <= MAX_STA. > > regards, > dan carpenter > ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: rtl8188eu: Fix off-by-one in core/rtw_mlme_ext.c
On Tue, Nov 27, 2018 at 08:41:53AM +, Yang Xiao wrote: > Okay. I can send a patch to revert ef9209b642f. > > But, can you make sure that the condition "(pstapriv->sta_aid[pstat->aid > - 1] == NULL)" can satisfies in the for loop? ->max_num_sta is either set in _rtw_init_sta_priv() or rtw_set_beacon(). You can see that it's <= MAX_STA. regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: rtl8188eu: Fix off-by-one in core/rtw_mlme_ext.c
Okay. I can send a patch to revert ef9209b642f.
But, can you make sure that the condition "(pstapriv->sta_aid[pstat->aid
- 1] == NULL)" can satisfies in the for loop?
Young
On 2018/11/27 16:34, Dan Carpenter wrote:
> On Tue, Nov 27, 2018 at 08:29:05AM +, Yang Xiao wrote:
>> Hi,
>>
>> See commit ef9209b642f ("staging: rtl8723bs: Fix indenting errors and an
>> off-by-one mistake in core/rtw_mlme_ext.c") for detail.
>>
>> I don't know how can you make sure that line 3254 can be true in the for
>> loop. If the condition never satisfies, then there is an off-by-one
>> access in line 3267.
>>
>> If you can prove it, then the patch is unnecessary.
>>
> Ugh... Patch ef9209b642f isn't right. :(
>
> Do you want to send a patch to change that back to the way it was. If
> not then I can do it.
>
> regards,
> dan carpenter
>
>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: rtl8188eu: Fix off-by-one in core/rtw_mlme_ext.c
On Tue, Nov 27, 2018 at 08:29:05AM +, Yang Xiao wrote:
> Hi,
>
> See commit ef9209b642f ("staging: rtl8723bs: Fix indenting errors and an
> off-by-one mistake in core/rtw_mlme_ext.c") for detail.
>
> I don't know how can you make sure that line 3254 can be true in the for
> loop. If the condition never satisfies, then there is an off-by-one
> access in line 3267.
>
> If you can prove it, then the patch is unnecessary.
>
Ugh... Patch ef9209b642f isn't right. :(
Do you want to send a patch to change that back to the way it was. If
not then I can do it.
regards,
dan carpenter
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: rtl8188eu: Fix off-by-one in core/rtw_mlme_ext.c
Hi,
See commit ef9209b642f ("staging: rtl8723bs: Fix indenting errors and an
off-by-one mistake in core/rtw_mlme_ext.c") for detail.
I don't know how can you make sure that line 3254 can be true in the for
loop. If the condition never satisfies, then there is an off-by-one
access in line 3267.
If you can prove it, then the patch is unnecessary.
Young
On 2018/11/27 16:15, Dan Carpenter wrote:
> The original code is OK.
>
> On Tue, Nov 27, 2018 at 07:29:07AM +, Yang Xiao wrote:
>> From: Young_X
>>
>> The error at line 3267 was the result of an off-by-one error in
>> a for loop in line 3253.
>> If condition in line 3254 never satisfies, then the value of
>> pstat->aid is NUM_STA+1. This will lead to out-of-bound access
>> in line 3267.
> It's best to avoid line numbers in the commit message unless you paste
> the actual code, because sometimes people will be using different line
> numbers from you.
>
>> Signed-off-by: Young_X
>> ---
>> drivers/staging/rtl8188eu/core/rtw_mlme_ext.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/staging/rtl8188eu/core/rtw_mlme_ext.c
>> b/drivers/staging/rtl8188eu/core/rtw_mlme_ext.c
>> index 6790b840..0854adc 100644
>> --- a/drivers/staging/rtl8188eu/core/rtw_mlme_ext.c
>> +++ b/drivers/staging/rtl8188eu/core/rtw_mlme_ext.c
>> @@ -3250,7 +3250,7 @@ static unsigned int OnAssocReq(struct adapter
>> *padapter,
>> if (pstat->aid > 0) {
>> DBG_88E(" old AID %d\n", pstat->aid);
>> } else {
>> -for (pstat->aid = 1; pstat->aid <= NUM_STA; pstat->aid++)
>> +for (pstat->aid = 1; pstat->aid < NUM_STA; pstat->aid++)
> drivers/staging/rtl8188eu/core/rtw_mlme_ext.c
>3249 /* get a unique AID */
>3250 if (pstat->aid > 0) {
>3251 DBG_88E(" old AID %d\n", pstat->aid);
>3252 } else {
>3253 for (pstat->aid = 1; pstat->aid <= NUM_STA;
> pstat->aid++)
>3254 if (pstapriv->sta_aid[pstat->aid - 1] ==
> NULL)
>3255 break;
>3256
>3257 /* if (pstat->aid > NUM_STA) { */
>
> This comment is key. pstapriv->max_num_sta is always less than or equal
> to NUM_STA.
>
>3258 if (pstat->aid > pstapriv->max_num_sta) {
>3259 pstat->aid = 0;
>3260
>3261 DBG_88E(" no room for more AIDs\n");
>3262
>3263 status =
> WLAN_STATUS_AP_UNABLE_TO_HANDLE_NEW_STA;
>3264
>3265 goto OnAssocReqFail;
>3266 } else {
>3267 pstapriv->sta_aid[pstat->aid - 1] = pstat;
>^^
> So this is fine.
>
>3268 DBG_88E("allocate new AID=(%d)\n",
> pstat->aid);
>3269 }
>3270 }
>3271
>
> regards,
> dan carpenter
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 2/2] media: cedrus: Add HEVC/H.265 decoding support
Hi!
On Fri, Nov 23, 2018 at 02:02:09PM +0100, Paul Kocialkowski wrote:
> This introduces support for HEVC/H.265 to the Cedrus VPU driver, with
> both uni-directional and bi-directional prediction modes supported.
>
> Field-coded (interlaced) pictures, custom quantization matrices and
> 10-bit output are not supported at this point.
>
> Signed-off-by: Paul Kocialkowski
Output from checkpatch:
total: 0 errors, 68 warnings, 14 checks, 999 lines checked
> +/*
> + * Note: Neighbor info buffer size is apparently doubled for H6, which may be
> + * related to 10 bit H265 support.
> + */
> +#define CEDRUS_H265_NEIGHBOR_INFO_BUF_SIZE (397 * SZ_1K)
> +#define CEDRUS_H265_ENTRY_POINTS_BUF_SIZE(4 * SZ_1K)
> +#define CEDRUS_H265_MV_COL_BUF_UNIT_CTB_SIZE 160
Having some information on where this is coming from would be useful.
> +static void cedrus_h265_sram_write_data(struct cedrus_dev *dev, u32 *data,
Since the data pointer is pretty much an opaque structure, you should
have a void pointer here, that would avoid the type casting you're
doing when calling that function.
> + unsigned int count)
> +{
> + while (count--)
> + cedrus_write(dev, VE_DEC_H265_SRAM_DATA, *data++);
> +}
> +
> +static inline dma_addr_t cedrus_h265_frame_info_mv_col_buf_addr(
> + struct cedrus_ctx *ctx, unsigned int index, unsigned int field)
> +{
> + return ctx->codec.h265.mv_col_buf_addr + index *
> +ctx->codec.h265.mv_col_buf_unit_size +
> +field * ctx->codec.h265.mv_col_buf_unit_size / 2;
> +}
> +
> +static void cedrus_h265_frame_info_write_single(struct cedrus_dev *dev,
> + unsigned int index,
> + bool field_pic,
> + u32 pic_order_cnt[],
> + dma_addr_t mv_col_buf_addr[],
> + dma_addr_t dst_luma_addr,
> + dma_addr_t dst_chroma_addr)
> +{
> + u32 offset = VE_DEC_H265_SRAM_OFFSET_FRAME_INFO +
> + VE_DEC_H265_SRAM_OFFSET_FRAME_INFO_UNIT * index;
> + struct cedrus_h265_sram_frame_info frame_info = {
> + .top_pic_order_cnt = pic_order_cnt[0],
> + .bottom_pic_order_cnt = field_pic ? pic_order_cnt[1] :
> + pic_order_cnt[0],
> + .top_mv_col_buf_addr =
> + VE_DEC_H265_SRAM_DATA_ADDR_BASE(mv_col_buf_addr[0]),
> + .bottom_mv_col_buf_addr = field_pic ?
> + VE_DEC_H265_SRAM_DATA_ADDR_BASE(mv_col_buf_addr[1]) :
> + VE_DEC_H265_SRAM_DATA_ADDR_BASE(mv_col_buf_addr[0]),
> + .luma_addr = VE_DEC_H265_SRAM_DATA_ADDR_BASE(dst_luma_addr),
> + .chroma_addr = VE_DEC_H265_SRAM_DATA_ADDR_BASE(dst_chroma_addr),
> + };
> + unsigned int count = sizeof(frame_info) / sizeof(u32);
> +
> + cedrus_h265_sram_write_offset(dev, offset);
> + cedrus_h265_sram_write_data(dev, (u32 *)&frame_info, count);
Usually, any generic write function will have its size passed in bytes.
> +}
> +
> +static void cedrus_h265_frame_info_write_dpb(struct cedrus_ctx *ctx,
> + const struct v4l2_hevc_dpb_entry
> *dpb,
> + u8 num_active_dpb_entries)
> +{
> + struct cedrus_dev *dev = ctx->dev;
> + struct vb2_queue *cap_q = &ctx->fh.m2m_ctx->cap_q_ctx.q;
> + unsigned int i;
> +
> + for (i = 0; i < num_active_dpb_entries; i++) {
> + dma_addr_t dst_luma_addr, dst_chroma_addr;
> + dma_addr_t mv_col_buf_addr[2];
> + u32 pic_order_cnt[2];
> + int buffer_index = vb2_find_tag(cap_q, dpb[i].buffer_tag, 0);
> +
> + dst_luma_addr = cedrus_dst_buf_addr(ctx, buffer_index, 0) -
> + PHYS_OFFSET;
> + dst_chroma_addr = cedrus_dst_buf_addr(ctx, buffer_index, 1) -
> + PHYS_OFFSET;
> + mv_col_buf_addr[0] = cedrus_h265_frame_info_mv_col_buf_addr(ctx,
> + buffer_index, 0) - PHYS_OFFSET;
The PHYS_OFFSET part should be part of
cedrus_h265_frame_info_mv_col_buf_addr.
> + pic_order_cnt[0] = dpb[i].pic_order_cnt[0];
> +
> + if (dpb[i].field_pic) {
> + mv_col_buf_addr[1] =
> + cedrus_h265_frame_info_mv_col_buf_addr(ctx,
> + buffer_index, 1) - PHYS_OFFSET;
> + pic_order_cnt[1] = dpb[i].pic_order_cnt[1];
> + }
> +
> + cedrus_h265_frame_info_write_single(dev, i, dpb[i].field_pic,
> + pic_order_cnt,
> + mv_col_buf_addr,
> +
Re: [PATCH] staging: rtl8188eu: Fix off-by-one in core/rtw_mlme_ext.c
The original code is OK.
On Tue, Nov 27, 2018 at 07:29:07AM +, Yang Xiao wrote:
> From: Young_X
>
> The error at line 3267 was the result of an off-by-one error in
> a for loop in line 3253.
> If condition in line 3254 never satisfies, then the value of
> pstat->aid is NUM_STA+1. This will lead to out-of-bound access
> in line 3267.
It's best to avoid line numbers in the commit message unless you paste
the actual code, because sometimes people will be using different line
numbers from you.
> Signed-off-by: Young_X
> ---
> drivers/staging/rtl8188eu/core/rtw_mlme_ext.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/staging/rtl8188eu/core/rtw_mlme_ext.c
> b/drivers/staging/rtl8188eu/core/rtw_mlme_ext.c
> index 6790b840..0854adc 100644
> --- a/drivers/staging/rtl8188eu/core/rtw_mlme_ext.c
> +++ b/drivers/staging/rtl8188eu/core/rtw_mlme_ext.c
> @@ -3250,7 +3250,7 @@ static unsigned int OnAssocReq(struct adapter *padapter,
> if (pstat->aid > 0) {
> DBG_88E(" old AID %d\n", pstat->aid);
> } else {
> - for (pstat->aid = 1; pstat->aid <= NUM_STA; pstat->aid++)
> + for (pstat->aid = 1; pstat->aid < NUM_STA; pstat->aid++)
drivers/staging/rtl8188eu/core/rtw_mlme_ext.c
3249 /* get a unique AID */
3250 if (pstat->aid > 0) {
3251 DBG_88E(" old AID %d\n", pstat->aid);
3252 } else {
3253 for (pstat->aid = 1; pstat->aid <= NUM_STA;
pstat->aid++)
3254 if (pstapriv->sta_aid[pstat->aid - 1] == NULL)
3255 break;
3256
3257 /* if (pstat->aid > NUM_STA) { */
This comment is key. pstapriv->max_num_sta is always less than or equal
to NUM_STA.
3258 if (pstat->aid > pstapriv->max_num_sta) {
3259 pstat->aid = 0;
3260
3261 DBG_88E(" no room for more AIDs\n");
3262
3263 status =
WLAN_STATUS_AP_UNABLE_TO_HANDLE_NEW_STA;
3264
3265 goto OnAssocReqFail;
3266 } else {
3267 pstapriv->sta_aid[pstat->aid - 1] = pstat;
^^
So this is fine.
3268 DBG_88E("allocate new AID=(%d)\n", pstat->aid);
3269 }
3270 }
3271
regards,
dan carpenter
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
