Re: [PATCH 0/5] Some cleanup patches for drivers/staging/rtl8723au/core/rtw_mlme.c
On 06-09-2016 12:00:37, Jes Sorensen wrote: > Matthias Beyer writes: > > This patchset fixes some errors and warnings reported by checkpatch.pl. > > > > Matthias Beyer (5): > > drivers: staging: rtl8723au: core: Fix checkpatch.pl errors > > drivers: staging: rtl8723au: core: simplify if-break-else > > drivers: staging: rtl8723au: core: Refactor pointless branching > > drivers: staging: rtl8723au: core: Fix "space prohibited" warning > > drivers: staging: rtl8723au: core: Fix indentation > > > > drivers/staging/rtl8723au/core/rtw_mlme.c | 72 > > ++- > > 1 file changed, 33 insertions(+), 39 deletions(-) > > Nothing wrong with these patches, however I intend to post a patch to > remove this driver soon, so it's kind of a waste of your time to spend > too many cycles on it. > Alright, thanks for telling me, I won't waste any more time on it. Besides that - thanks for telling me that the patchset is okay as-is. That keeps me motivated! -- Mit freundlichen Grüßen, Kind regards, Matthias Beyer Proudly sent with mutt. Happily signed with gnupg. signature.asc Description: PGP signature ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 1/3] storvsc: use tagged SRB requests if supported by the device
On Tue, Sep 06, 2016 at 02:25:41PM -0700, Long Li wrote:
> From: Long Li
>
> Properly set SRB flags when hosting device supports tagged queuing. This
> patch improves the performance on Fiber Channel disks.
ENOSIGNEDOFF and please use checkpatch.pl on the patch.
>
> ---
> drivers/scsi/storvsc_drv.c | 8
> 1 file changed, 8 insertions(+)
>
> diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
> index 8ccfc9e..a8f3e4c 100644
> --- a/drivers/scsi/storvsc_drv.c
> +++ b/drivers/scsi/storvsc_drv.c
> @@ -136,6 +136,8 @@ struct hv_fc_wwn_packet {
> #define SRB_FLAGS_PORT_DRIVER_RESERVED 0x0F00
> #define SRB_FLAGS_CLASS_DRIVER_RESERVED 0xF000
>
> +#define SP_UNTAGGED ((unsigned char) ~0)
> +#define SRB_SIMPLE_TAG_REQUEST 0x20
>
> /*
> * Platform neutral description of a scsi request -
> @@ -1451,6 +1453,12 @@ static int storvsc_queuecommand(struct Scsi_Host
> *host, struct scsi_cmnd *scmnd)
> vm_srb->win8_extension.srb_flags |=
> SRB_FLAGS_DISABLE_SYNCH_TRANSFER;
>
> + if(scmnd->device->tagged_supported) {
> + vm_srb->win8_extension.srb_flags |=
> (SRB_FLAGS_QUEUE_ACTION_ENABLE | SRB_FLAGS_NO_QUEUE_FREEZE);
> + vm_srb->win8_extension.queue_tag = SP_UNTAGGED;
> + vm_srb->win8_extension.queue_action = SRB_SIMPLE_TAG_REQUEST;
> + }
> +
> /* Build the SRB */
> switch (scmnd->sc_data_direction) {
> case DMA_TO_DEVICE:
> --
> 1.8.5.6
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Johannes Thumshirn Storage
jthumsh...@suse.de+49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCHv2 1/4] staging: android: ion: Drop heap type masks
On Tue, Sep 06, 2016 at 03:16:52PM -0700, Laura Abbott wrote:
On 09/05/2016 04:20 AM, Brian Starkey wrote:
Hi,
On Fri, Sep 02, 2016 at 12:36:25PM -0700, Laura Abbott wrote:
On 09/02/2016 06:41 AM, Brian Starkey wrote:
Hi Laura,
On Thu, Sep 01, 2016 at 03:40:41PM -0700, Laura Abbott wrote:
There is no advantage to having heap types be a mask. The ion client has
long since dropped the mask. Drop the notion of heap type masks as well.
I know this is the same patch you sent last time, so sorry for not
picking this up then - but I'm curious what "The" ion client is here?
ion_client_create used to take a mask to indicate what heap types it
could allocate from. This hasn't been the case since 2bb9f5034ec7
("gpu: ion: Remove heapmask from client"). "The ion client" probably
should have been "struct ion_client"
Ah I see, the in-kernel ion_client. Sorry, I completely forgot that
even existed (because it's totally useless - how is a driver meant to
find the global ion_device?)
Our ion client(s) certainly still use these masks, and it's still
used as a mask within ion itself - even if the relationship between a
mask and a heap type has been somewhat lost.
Where is it used in Ion? I don't see it in tree unless I missed something
and I'm not eager to keep this around for out of tree code. What's the
actual use for this?
You're certainly right that these heap-ID-to-allocation-mask macros
are unused in the kernel, but I don't really see the reason for
removing them - they are convenient (for now).
Example: I'm using the dummy ion driver, and I want to allocate from
the SYSTEM_CONTIG heap - the ION_HEAP_SYSTEM_CONTIG_MASK gives me the
exact mask I need for that.
It seems your opinion is that heap-IDs are already, and should be,
completely decoupled from their type. That sounds like a good idea to
me, but it's not true (yet) - again check out the dummy driver.
Good point, I need to clean up the dummy driver to stop using heap
types as the id ;)
I get that it's convenient but it's a bad practice to conflate the
namespaces.
At the moment, heap-IDs are assigned by ion drivers in any way they
see fit. For as long as that stays the case there's always going to
be heap-masks hard-coded in UAPI kernel headers (in-tree or not), so
removing these particular masks seems a bit fruitless.
It's not fruitless, the concept of type as mask makes no sense. They
are two different name spaces and I've found Ion users have a hard
time keeping them separate and pass in the heap type mask when using
non dummy
You can add me to that group :-)
I'd rather see driver-assigned heap-IDs disappear completely, and have
them assigned by ion core from an idr or something. At that point
these macros really *are* meaningless, and I'd be totally fine with
removing them (and userspace won't be able to depend on hard-coded
allocation masks any more - it will have to use the query ioctl,
which I assume is the whole point?).
Ideally yes we'd be able to get rid of the hard coded device IDs.
I consider the query ioctl a stepping stone to that, depending on
how enthusiastic people are about Ion.
IMO it's not the right time to remove these macros, because they still
have meaning and usefulness.
I still think they should be deleted to avoid namespace polution.
If you nuke type-as-ID in dummy, and put a clear comment on the
ion_heap_type enum stating that heap-type and heap-ID are strictly
different, then I'm happy.
I think without that there'll still be confusion.
Thanks!
Brian
Cheers,
Brian
Thanks,
Brian
Signed-off-by: Laura Abbott
---
drivers/staging/android/uapi/ion.h | 6 --
1 file changed, 6 deletions(-)
diff --git a/drivers/staging/android/uapi/ion.h
b/drivers/staging/android/uapi/ion.h
index 0a8e40f..a9c4e8b 100644
--- a/drivers/staging/android/uapi/ion.h
+++ b/drivers/staging/android/uapi/ion.h
@@ -44,14 +44,8 @@ enum ion_heap_type {
* must be last so device specific heaps always
* are at the end of this enum
*/
-ION_NUM_HEAPS = 16,
};
-#define ION_HEAP_SYSTEM_MASK(1 << ION_HEAP_TYPE_SYSTEM)
-#define ION_HEAP_SYSTEM_CONTIG_MASK(1 << ION_HEAP_TYPE_SYSTEM_CONTIG)
-#define ION_HEAP_CARVEOUT_MASK(1 << ION_HEAP_TYPE_CARVEOUT)
-#define ION_HEAP_TYPE_DMA_MASK(1 << ION_HEAP_TYPE_DMA)
-
#define ION_NUM_HEAP_IDS(sizeof(unsigned int) * 8)
/**
--
2.7.4
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 0/2] Drivers: hv: vmbus: Some miscellaneous fixes
From: K. Y. Srinivasan Some miscellaneous fixes. Dexuan Cui (1): Drivers: hv: vmbus: suppress some "hv_vmbus: Unknown GUID" warnings Stephen Hemminger (1): Driver: hv: vmbus: Make mmio resource local drivers/hv/channel_mgmt.c | 26 -- drivers/hv/vmbus_drv.c|4 ++-- include/linux/hyperv.h| 21 + 3 files changed, 47 insertions(+), 4 deletions(-) -- 1.7.4.1 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 2/2] Drivers: hv: vmbus: suppress some "hv_vmbus: Unknown GUID" warnings
From: Dexuan Cui
Some VMBus devices are not needed by Linux guest[1][2], and, VMBus channels
of Hyper-V Sockets don't really mean usual synthetic devices, so let's
suppress the warnings for them.
[1] https://support.microsoft.com/en-us/kb/2925727
[2] https://msdn.microsoft.com/en-us/library/jj980180(v=winembedded.81).aspx
Signed-off-by: Dexuan Cui
Signed-off-by: K. Y. Srinivasan
---
drivers/hv/channel_mgmt.c | 26 --
include/linux/hyperv.h| 21 +
2 files changed, 45 insertions(+), 2 deletions(-)
diff --git a/drivers/hv/channel_mgmt.c b/drivers/hv/channel_mgmt.c
index bbd812e..759ba4d 100644
--- a/drivers/hv/channel_mgmt.c
+++ b/drivers/hv/channel_mgmt.c
@@ -139,10 +139,32 @@ static const struct vmbus_device vmbus_devs[] = {
},
};
-static u16 hv_get_dev_type(const uuid_le *guid)
+static const struct {
+ uuid_le guid;
+} vmbus_unsupported_devs[] = {
+ { HV_AVMA1_GUID },
+ { HV_AVMA2_GUID },
+ { HV_RDV_GUID },
+};
+
+static bool is_unsupported_vmbus_devs(const uuid_le *guid)
+{
+ int i;
+
+ for (i = 0; i < ARRAY_SIZE(vmbus_unsupported_devs); i++)
+ if (!uuid_le_cmp(*guid, vmbus_unsupported_devs[i].guid))
+ return true;
+ return false;
+}
+
+static u16 hv_get_dev_type(const struct vmbus_channel *channel)
{
+ const uuid_le *guid = &channel->offermsg.offer.if_type;
u16 i;
+ if (is_hvsock_channel(channel) || is_unsupported_vmbus_devs(guid))
+ return HV_UNKOWN;
+
for (i = HV_IDE; i < HV_UNKOWN; i++) {
if (!uuid_le_cmp(*guid, vmbus_devs[i].guid))
return i;
@@ -426,7 +448,7 @@ static void vmbus_process_offer(struct vmbus_channel
*newchannel)
goto err_free_chan;
}
- dev_type = hv_get_dev_type(&newchannel->offermsg.offer.if_type);
+ dev_type = hv_get_dev_type(newchannel);
if (dev_type == HV_NIC)
set_channel_signal_state(newchannel, HV_SIGNAL_POLICY_EXPLICIT);
diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
index 613074e..430619a 100644
--- a/include/linux/hyperv.h
+++ b/include/linux/hyperv.h
@@ -1315,6 +1315,27 @@ u64 hv_do_hypercall(u64 control, void *input, void
*output);
0x80, 0x2e, 0x27, 0xed, 0xe1, 0x9f)
/*
+ * Linux doesn't support the 3 devices: the first two are for
+ * Automatic Virtual Machine Activation, and the third is for
+ * Remote Desktop Virtualization.
+ * {f8e65716-3cb3-4a06-9a60-1889c5cccab5}
+ * {3375baf4-9e15-4b30-b765-67acb10d607b}
+ * {276aacf4-ac15-426c-98dd-7521ad3f01fe}
+ */
+
+#define HV_AVMA1_GUID \
+ .guid = UUID_LE(0xf8e65716, 0x3cb3, 0x4a06, 0x9a, 0x60, \
+ 0x18, 0x89, 0xc5, 0xcc, 0xca, 0xb5)
+
+#define HV_AVMA2_GUID \
+ .guid = UUID_LE(0x3375baf4, 0x9e15, 0x4b30, 0xb7, 0x65, \
+ 0x67, 0xac, 0xb1, 0x0d, 0x60, 0x7b)
+
+#define HV_RDV_GUID \
+ .guid = UUID_LE(0x276aacf4, 0xac15, 0x426c, 0x98, 0xdd, \
+ 0x75, 0x21, 0xad, 0x3f, 0x01, 0xfe)
+
+/*
* Common header for Hyper-V ICs
*/
--
1.7.4.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 1/2] Driver: hv: vmbus: Make mmio resource local
From: Stephen Hemminger
This fixes a sparse warning because hyperv_mmio resources
are only used in this one file and should be static.
Signed-off-by: Stephen Hemminger
Signed-off-by: K. Y. Srinivasan
---
drivers/hv/vmbus_drv.c |4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
index fedf629..6cbe074 100644
--- a/drivers/hv/vmbus_drv.c
+++ b/drivers/hv/vmbus_drv.c
@@ -105,8 +105,8 @@ static struct notifier_block hyperv_panic_block = {
static const char *fb_mmio_name = "fb_range";
static struct resource *fb_mmio;
-struct resource *hyperv_mmio;
-DEFINE_SEMAPHORE(hyperv_mmio_lock);
+static struct resource *hyperv_mmio;
+static DEFINE_SEMAPHORE(hyperv_mmio_lock);
static int vmbus_exists(void)
{
--
1.7.4.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] staging: ks7010: avoid dereferencing packet if it is null
From: Colin Ian King
Updating tx_bytes from packet->len if packet is null will cause
a null pointer dereference, so only update tx_bytes if it packet
is not null.
Signed-off-by: Colin Ian King
---
drivers/staging/ks7010/ks_wlan_net.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/staging/ks7010/ks_wlan_net.c
b/drivers/staging/ks7010/ks_wlan_net.c
index 1e21eb1..d69b4c9 100644
--- a/drivers/staging/ks7010/ks_wlan_net.c
+++ b/drivers/staging/ks7010/ks_wlan_net.c
@@ -3396,13 +3396,13 @@ void send_packet_complete(void *arg1, void *arg2)
DPRINTK(3, "\n");
- priv->nstats.tx_bytes += packet->len;
priv->nstats.tx_packets++;
if (netif_queue_stopped(priv->net_dev))
netif_wake_queue(priv->net_dev);
if (packet) {
+ priv->nstats.tx_bytes += packet->len;
dev_kfree_skb(packet);
packet = NULL;
}
--
2.9.3
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
RE: [PATCH 1/3] storvsc: use tagged SRB requests if supported by the device
> -Original Message-
> From: Johannes Thumshirn [mailto:jthumsh...@suse.de]
> Sent: Wednesday, September 7, 2016 12:47 AM
> To: Long Li
> Cc: KY Srinivasan ; Haiyang Zhang
> ; James E.J. Bottomley
> ; Martin K. Petersen
> ; de...@linuxdriverproject.org; linux-
> s...@vger.kernel.org; linux-ker...@vger.kernel.org; Long Li
>
> Subject: Re: [PATCH 1/3] storvsc: use tagged SRB requests if supported by
> the device
>
> On Tue, Sep 06, 2016 at 02:25:41PM -0700, Long Li wrote:
> > From: Long Li
> >
> > Properly set SRB flags when hosting device supports tagged queuing. This
> patch improves the performance on Fiber Channel disks.
>
> ENOSIGNEDOFF and please use checkpatch.pl on the patch.
Thanks for pointing that out. I'll re-send the patches.
>
> >
> > ---
> > drivers/scsi/storvsc_drv.c | 8
> > 1 file changed, 8 insertions(+)
> >
> > diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
> > index 8ccfc9e..a8f3e4c 100644
> > --- a/drivers/scsi/storvsc_drv.c
> > +++ b/drivers/scsi/storvsc_drv.c
> > @@ -136,6 +136,8 @@ struct hv_fc_wwn_packet {
> > #define SRB_FLAGS_PORT_DRIVER_RESERVED 0x0F00
> > #define SRB_FLAGS_CLASS_DRIVER_RESERVED0xF000
> >
> > +#define SP_UNTAGGED((unsigned char) ~0)
> > +#define SRB_SIMPLE_TAG_REQUEST 0x20
> >
> > /*
> > * Platform neutral description of a scsi request - @@ -1451,6
> > +1453,12 @@ static int storvsc_queuecommand(struct Scsi_Host *host,
> struct scsi_cmnd *scmnd)
> > vm_srb->win8_extension.srb_flags |=
> > SRB_FLAGS_DISABLE_SYNCH_TRANSFER;
> >
> > + if(scmnd->device->tagged_supported) {
> > + vm_srb->win8_extension.srb_flags |=
> (SRB_FLAGS_QUEUE_ACTION_ENABLE | SRB_FLAGS_NO_QUEUE_FREEZE);
> > + vm_srb->win8_extension.queue_tag = SP_UNTAGGED;
> > + vm_srb->win8_extension.queue_action =
> SRB_SIMPLE_TAG_REQUEST;
> > + }
> > +
> > /* Build the SRB */
> > switch (scmnd->sc_data_direction) {
> > case DMA_TO_DEVICE:
> > --
> > 1.8.5.6
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-scsi"
> > in the body of a message to majord...@vger.kernel.org More
> majordomo
> > info at
> > https://na01.safelinks.protection.outlook.com/?url=http%3a%2f%2fvger.k
> > ernel.org%2fmajordomo-
> info.html&data=02%7c01%7clongli%40microsoft.com%
> >
> 7cdedd4c7ad4cf4955224d08d3d6f31f3d%7c72f988bf86f141af91ab2d7cd011db
> 47%
> >
> 7c1%7c0%7c636088312112339554&sdata=QvrOLvFjisQ4Nfz%2bkz1uyt7G7wh
> R7Uz7D
> > DlYMuc5VUM%3d
>
> --
> Johannes Thumshirn Storage
> jthumsh...@suse.de+49 911 74053 689
> SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
> GF: Felix Imendörffer, Jane Smithard, Graham Norton HRB 21284 (AG
> Nürnberg) Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76
> 0850
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCHv3 1/2] staging: android: ion: Pull out ion ioctls to a separate file
The number of Ion ioctls may continue to grow along with necessary
validation. Pull it out into a separate file for easier management
and review.
Signed-off-by: Laura Abbott
---
v3: Rebase to staging-next
---
drivers/staging/android/ion/Makefile| 3 +-
drivers/staging/android/ion/ion-ioctl.c | 144 ++
drivers/staging/android/ion/ion.c | 212 ++--
drivers/staging/android/ion/ion_priv.h | 91 ++
4 files changed, 245 insertions(+), 205 deletions(-)
create mode 100644 drivers/staging/android/ion/ion-ioctl.c
diff --git a/drivers/staging/android/ion/Makefile
b/drivers/staging/android/ion/Makefile
index 18cc2aa..376c2b2 100644
--- a/drivers/staging/android/ion/Makefile
+++ b/drivers/staging/android/ion/Makefile
@@ -1,4 +1,5 @@
-obj-$(CONFIG_ION) += ion.o ion_heap.o ion_page_pool.o ion_system_heap.o \
+obj-$(CONFIG_ION) += ion.o ion-ioctl.o ion_heap.o \
+ ion_page_pool.o ion_system_heap.o \
ion_carveout_heap.o ion_chunk_heap.o ion_cma_heap.o
obj-$(CONFIG_ION_TEST) += ion_test.o
ifdef CONFIG_COMPAT
diff --git a/drivers/staging/android/ion/ion-ioctl.c
b/drivers/staging/android/ion/ion-ioctl.c
new file mode 100644
index 000..341ba7d
--- /dev/null
+++ b/drivers/staging/android/ion/ion-ioctl.c
@@ -0,0 +1,144 @@
+/*
+ *
+ * Copyright (C) 2011 Google, Inc.
+ *
+ * This software is licensed under the terms of the GNU General Public
+ * License version 2, as published by the Free Software Foundation, and
+ * may be copied, distributed, and modified under those terms.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ */
+
+#include
+#include
+#include
+#include
+
+#include "ion.h"
+#include "ion_priv.h"
+#include "compat_ion.h"
+
+/* fix up the cases where the ioctl direction bits are incorrect */
+static unsigned int ion_ioctl_dir(unsigned int cmd)
+{
+ switch (cmd) {
+ case ION_IOC_SYNC:
+ case ION_IOC_FREE:
+ case ION_IOC_CUSTOM:
+ return _IOC_WRITE;
+ default:
+ return _IOC_DIR(cmd);
+ }
+}
+
+long ion_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
+{
+ struct ion_client *client = filp->private_data;
+ struct ion_device *dev = client->dev;
+ struct ion_handle *cleanup_handle = NULL;
+ int ret = 0;
+ unsigned int dir;
+
+ union {
+ struct ion_fd_data fd;
+ struct ion_allocation_data allocation;
+ struct ion_handle_data handle;
+ struct ion_custom_data custom;
+ } data;
+
+ dir = ion_ioctl_dir(cmd);
+
+ if (_IOC_SIZE(cmd) > sizeof(data))
+ return -EINVAL;
+
+ if (dir & _IOC_WRITE)
+ if (copy_from_user(&data, (void __user *)arg, _IOC_SIZE(cmd)))
+ return -EFAULT;
+
+ switch (cmd) {
+ case ION_IOC_ALLOC:
+ {
+ struct ion_handle *handle;
+
+ handle = ion_alloc(client, data.allocation.len,
+ data.allocation.align,
+ data.allocation.heap_id_mask,
+ data.allocation.flags);
+ if (IS_ERR(handle))
+ return PTR_ERR(handle);
+
+ data.allocation.handle = handle->id;
+
+ cleanup_handle = handle;
+ break;
+ }
+ case ION_IOC_FREE:
+ {
+ struct ion_handle *handle;
+
+ mutex_lock(&client->lock);
+ handle = ion_handle_get_by_id_nolock(client,
data.handle.handle);
+ if (IS_ERR(handle)) {
+ mutex_unlock(&client->lock);
+ return PTR_ERR(handle);
+ }
+ ion_free_nolock(client, handle);
+ ion_handle_put_nolock(handle);
+ mutex_unlock(&client->lock);
+ break;
+ }
+ case ION_IOC_SHARE:
+ case ION_IOC_MAP:
+ {
+ struct ion_handle *handle;
+
+ handle = ion_handle_get_by_id(client, data.handle.handle);
+ if (IS_ERR(handle))
+ return PTR_ERR(handle);
+ data.fd.fd = ion_share_dma_buf_fd(client, handle);
+ ion_handle_put(handle);
+ if (data.fd.fd < 0)
+ ret = data.fd.fd;
+ break;
+ }
+ case ION_IOC_IMPORT:
+ {
+ struct ion_handle *handle;
+
+ handle = ion_import_dma_buf_fd(client, data.fd.fd);
+ if (IS_ERR(handle))
+ ret = PTR_ERR(handle);
+ else
+
[PATCHv3 0/2] New Ion query ioctl
Hi, This is v3 of the previous series. The scope continues to shrink. The ABI ioctl was dropped after discussion about how it creates more problems than it actually solves. This is mostly a rebase to staging-next with some refactoring from not having the ABI ioctl. There was some discussion about ion_dummy cleanup but I've decided to have that be a separate patch. Laura Abbott (2): staging: android: ion: Pull out ion ioctls to a separate file staging: android: ion: Add ioctl to query available heaps drivers/staging/android/ion/Makefile| 3 +- drivers/staging/android/ion/ion-ioctl.c | 177 + drivers/staging/android/ion/ion.c | 227 ++-- drivers/staging/android/ion/ion_priv.h | 94 + drivers/staging/android/uapi/ion.h | 39 ++ 5 files changed, 349 insertions(+), 191 deletions(-) create mode 100644 drivers/staging/android/ion/ion-ioctl.c -- 2.7.4 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCHv3 2/2] staging: android: ion: Add ioctl to query available heaps
Ion clients currently lack a good method to determine what
heaps are available and what ids they map to. This leads
to tight coupling between user and kernel space and headaches.
Add a query ioctl to let userspace know the availability of
heaps.
Signed-off-by: Laura Abbott
---
v3: Include some refactoring that was previously part of the dropped ABI ioctl.
Use u64_to_user_ptr to avoid warnings and ugly double casts. General rebase
due to dropped ABI ioctl.
---
drivers/staging/android/ion/ion-ioctl.c | 53 ++---
drivers/staging/android/ion/ion.c | 43 ++
drivers/staging/android/ion/ion_priv.h | 3 ++
drivers/staging/android/uapi/ion.h | 39
4 files changed, 128 insertions(+), 10 deletions(-)
diff --git a/drivers/staging/android/ion/ion-ioctl.c
b/drivers/staging/android/ion/ion-ioctl.c
index 341ba7d..7e7431d 100644
--- a/drivers/staging/android/ion/ion-ioctl.c
+++ b/drivers/staging/android/ion/ion-ioctl.c
@@ -22,6 +22,31 @@
#include "ion_priv.h"
#include "compat_ion.h"
+union ion_ioctl_arg {
+ struct ion_fd_data fd;
+ struct ion_allocation_data allocation;
+ struct ion_handle_data handle;
+ struct ion_custom_data custom;
+ struct ion_heap_query query;
+};
+
+static int validate_ioctl_arg(unsigned int cmd, union ion_ioctl_arg *arg)
+{
+ int ret = 0;
+
+ switch (cmd) {
+ case ION_IOC_HEAP_QUERY:
+ ret = arg->query.reserved0 != 0;
+ ret |= arg->query.reserved1 != 0;
+ ret |= arg->query.reserved2 != 0;
+ break;
+ default:
+ break;
+ }
+
+ return ret ? -EINVAL : 0;
+}
+
/* fix up the cases where the ioctl direction bits are incorrect */
static unsigned int ion_ioctl_dir(unsigned int cmd)
{
@@ -42,22 +67,27 @@ long ion_ioctl(struct file *filp, unsigned int cmd,
unsigned long arg)
struct ion_handle *cleanup_handle = NULL;
int ret = 0;
unsigned int dir;
-
- union {
- struct ion_fd_data fd;
- struct ion_allocation_data allocation;
- struct ion_handle_data handle;
- struct ion_custom_data custom;
- } data;
+ union ion_ioctl_arg data;
dir = ion_ioctl_dir(cmd);
if (_IOC_SIZE(cmd) > sizeof(data))
return -EINVAL;
- if (dir & _IOC_WRITE)
- if (copy_from_user(&data, (void __user *)arg, _IOC_SIZE(cmd)))
- return -EFAULT;
+ /*
+* The copy_from_user is unconditional here for both read and write
+* to do the validate. If there is no write for the ioctl, the
+* buffer is cleared
+*/
+ if (copy_from_user(&data, (void __user *)arg, _IOC_SIZE(cmd)))
+ return -EFAULT;
+
+ ret = validate_ioctl_arg(cmd, &data);
+ if (WARN_ON_ONCE(ret))
+ return ret;
+
+ if (!(dir & _IOC_WRITE))
+ memset(&data, 0, sizeof(data));
switch (cmd) {
case ION_IOC_ALLOC:
@@ -129,6 +159,9 @@ long ion_ioctl(struct file *filp, unsigned int cmd,
unsigned long arg)
data.custom.arg);
break;
}
+ case ION_IOC_HEAP_QUERY:
+ ret = ion_query_heaps(client, &data.query);
+ break;
default:
return -ENOTTY;
}
diff --git a/drivers/staging/android/ion/ion.c
b/drivers/staging/android/ion/ion.c
index a92804f..396ded5 100644
--- a/drivers/staging/android/ion/ion.c
+++ b/drivers/staging/android/ion/ion.c
@@ -1159,6 +1159,48 @@ int ion_sync_for_device(struct ion_client *client, int
fd)
return 0;
}
+int ion_query_heaps(struct ion_client *client, struct ion_heap_query *query)
+{
+ struct ion_device *dev = client->dev;
+ struct ion_heap_data __user *buffer = u64_to_user_ptr(query->heaps);
+ int ret = -EINVAL, cnt = 0, max_cnt;
+ struct ion_heap *heap;
+ struct ion_heap_data hdata;
+
+ memset(&hdata, 0, sizeof(hdata));
+
+ down_read(&dev->lock);
+ if (!buffer) {
+ query->cnt = dev->heap_cnt;
+ ret = 0;
+ goto out;
+ }
+
+ if (query->cnt <= 0)
+ goto out;
+
+ max_cnt = query->cnt;
+
+ plist_for_each_entry(heap, &dev->heaps, node) {
+ strncpy(hdata.name, heap->name, MAX_HEAP_NAME);
+ hdata.name[sizeof(hdata.name) - 1] = '\0';
+ hdata.type = heap->type;
+ hdata.heap_id = heap->id;
+
+ ret = copy_to_user(&buffer[cnt],
+ &hdata, sizeof(hdata));
+
+ cnt++;
+ if (cnt >= max_cnt)
+ break;
+ }
+
+ query->cnt = cnt;
+out:
+ up_read(&dev->lock);
+ return ret;
+}
+
static int ion_release(struct inode *inode, struct file
Re: [Linaro-mm-sig] [PATCHv3 2/2] staging: android: ion: Add ioctl to query available heaps
On Wednesday, September 7, 2016 11:49:59 AM CEST Laura Abbott wrote: > - if (dir & _IOC_WRITE) > - if (copy_from_user(&data, (void __user *)arg, _IOC_SIZE(cmd))) > - return -EFAULT; > + /* > + * The copy_from_user is unconditional here for both read and write > + * to do the validate. If there is no write for the ioctl, the > + * buffer is cleared > + */ > + if (copy_from_user(&data, (void __user *)arg, _IOC_SIZE(cmd))) > + return -EFAULT; > + > + ret = validate_ioctl_arg(cmd, &data); > + if (WARN_ON_ONCE(ret)) > + return ret; I noticed that the WARN_ON_ONCE warns about invalid user input, but I think we tend to normally just use WARN_ON for things that go wrong inside of the kernel or in hardware. Maybe better use printk_once() or printk_ratelimited. Is there any noticeable overhead in always copying the structure? copy_from_user() can be a bit slow depending on debugging or security features, and it seems unnecessary if the validation is only done for one of the commands. Otherwise the patch looks good to me. Arnd ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [Linaro-mm-sig] [PATCHv3 1/2] staging: android: ion: Pull out ion ioctls to a separate file
On Wednesday, September 7, 2016 11:49:58 AM CEST Laura Abbott wrote: > The number of Ion ioctls may continue to grow along with necessary > validation. Pull it out into a separate file for easier management > and review. > > Signed-off-by: Laura Abbott > Acked-by: Arnd Bergmann ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [Linaro-mm-sig] [PATCHv3 2/2] staging: android: ion: Add ioctl to query available heaps
On 09/07/2016 12:37 PM, Arnd Bergmann wrote: On Wednesday, September 7, 2016 11:49:59 AM CEST Laura Abbott wrote: - if (dir & _IOC_WRITE) - if (copy_from_user(&data, (void __user *)arg, _IOC_SIZE(cmd))) - return -EFAULT; + /* +* The copy_from_user is unconditional here for both read and write +* to do the validate. If there is no write for the ioctl, the +* buffer is cleared +*/ + if (copy_from_user(&data, (void __user *)arg, _IOC_SIZE(cmd))) + return -EFAULT; + + ret = validate_ioctl_arg(cmd, &data); + if (WARN_ON_ONCE(ret)) + return ret; I noticed that the WARN_ON_ONCE warns about invalid user input, but I think we tend to normally just use WARN_ON for things that go wrong inside of the kernel or in hardware. Maybe better use printk_once() or printk_ratelimited. Sure, the error code should hopefully be enough of a hint to userspace to maybe check the log. Is there any noticeable overhead in always copying the structure? copy_from_user() can be a bit slow depending on debugging or security features, and it seems unnecessary if the validation is only done for one of the commands. Good point. It made sense with some of the other ioctls (specifically the ABI) but isn't necessary now. We can evaluate later when other ioctls get added. Otherwise the patch looks good to me. Arnd Thanks! Laura ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
