* [PATCH char-misc-linus 1/5] misc: mic: Change mic_notify(...) to return true.
2013-11-26 18:04 [PATCH char-misc-linus 0/5] misc: mic: Fixes for 3.13-rc2 Ashutosh Dixit
@ 2013-11-26 18:08 ` Ashutosh Dixit
2013-11-26 18:11 ` [PATCH char-misc-linus 2/5] misc: mic: Minor bug fix in 'retry' loops Ashutosh Dixit
` (3 subsequent siblings)
4 siblings, 0 replies; 9+ messages in thread
From: Ashutosh Dixit @ 2013-11-26 18:08 UTC (permalink / raw)
To: Greg Kroah-Hartman, Arnd Bergmann, linux-kernel, virtualization
Cc: Dasaratharaman Chandramouli, Ashutosh Dixit, Sudeep Dutt,
Nikhil Rao, Siva Krishna Kumar Reddy Yerramreddy, Caz Yokoyama,
Geert Uytterhoeven, Fengguang Wu
From: Sudeep Dutt <sudeep.dutt@intel.com>
virtqueue_{kick()/notify()} methods are required to return bool due to
API changes introduced in commit 5b1bf7cb673a.
Reported-by: Fengguang Wu <fengguang.wu@intel.com>
Reported-by: Geert Uytterhoeven <geert@linux-m68k.org>
Reviewed-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
Signed-off-by: Sudeep Dutt <sudeep.dutt@intel.com>
---
drivers/misc/mic/card/mic_virtio.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/misc/mic/card/mic_virtio.c b/drivers/misc/mic/card/mic_virtio.c
index 8aa42e7..1ebc51c 100644
--- a/drivers/misc/mic/card/mic_virtio.c
+++ b/drivers/misc/mic/card/mic_virtio.c
@@ -187,11 +187,12 @@ static void mic_reset(struct virtio_device *vdev)
/*
* The virtio_ring code calls this API when it wants to notify the Host.
*/
-static void mic_notify(struct virtqueue *vq)
+static bool mic_notify(struct virtqueue *vq)
{
struct mic_vdev *mvdev = vq->priv;
mic_send_intr(mvdev->mdev, mvdev->c2h_vdev_db);
+ return true;
}
static void mic_del_vq(struct virtqueue *vq, int n)
--
1.8.2.3
^ permalink raw reply related [flat|nested] 9+ messages in thread* [PATCH char-misc-linus 2/5] misc: mic: Minor bug fix in 'retry' loops.
2013-11-26 18:04 [PATCH char-misc-linus 0/5] misc: mic: Fixes for 3.13-rc2 Ashutosh Dixit
2013-11-26 18:08 ` [PATCH char-misc-linus 1/5] misc: mic: Change mic_notify(...) to return true Ashutosh Dixit
@ 2013-11-26 18:11 ` Ashutosh Dixit
2013-11-26 18:12 ` [PATCH char-misc-linus 3/5] misc: mic: Bug fix for sysfs poll usage Ashutosh Dixit
` (2 subsequent siblings)
4 siblings, 0 replies; 9+ messages in thread
From: Ashutosh Dixit @ 2013-11-26 18:11 UTC (permalink / raw)
To: Greg Kroah-Hartman, Arnd Bergmann, linux-kernel, virtualization
Cc: Dasaratharaman Chandramouli, Ashutosh Dixit, Sudeep Dutt,
Nikhil Rao, Siva Krishna Kumar Reddy Yerramreddy, Caz Yokoyama,
Michael Opdenacker
From: Sudeep Dutt <sudeep.dutt@intel.com>
The bug would result in incorrect 'retry' value being printed in debug
statements as well as dead code in mic_find_vqs(...) in
drivers/misc/mic/card/mic_virtio.c.
Reported-by: Michael Opdenacker <michael.opdenacker@free-electrons.com>
Reviewed-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
Signed-off-by: Sudeep Dutt <sudeep.dutt@intel.com>
---
drivers/misc/mic/card/mic_virtio.c | 8 ++++----
drivers/misc/mic/host/mic_virtio.c | 12 ++++++------
2 files changed, 10 insertions(+), 10 deletions(-)
diff --git a/drivers/misc/mic/card/mic_virtio.c b/drivers/misc/mic/card/mic_virtio.c
index 1ebc51c..4dce912 100644
--- a/drivers/misc/mic/card/mic_virtio.c
+++ b/drivers/misc/mic/card/mic_virtio.c
@@ -154,14 +154,14 @@ static void mic_reset_inform_host(struct virtio_device *vdev)
{
struct mic_vdev *mvdev = to_micvdev(vdev);
struct mic_device_ctrl __iomem *dc = mvdev->dc;
- int retry = 100, i;
+ int retry;
iowrite8(0, &dc->host_ack);
iowrite8(1, &dc->vdev_reset);
mic_send_intr(mvdev->mdev, mvdev->c2h_vdev_db);
/* Wait till host completes all card accesses and acks the reset */
- for (i = retry; i--;) {
+ for (retry = 100; retry--;) {
if (ioread8(&dc->host_ack))
break;
msleep(100);
@@ -310,7 +310,7 @@ static int mic_find_vqs(struct virtio_device *vdev, unsigned nvqs,
{
struct mic_vdev *mvdev = to_micvdev(vdev);
struct mic_device_ctrl __iomem *dc = mvdev->dc;
- int i, err, retry = 100;
+ int i, err, retry;
/* We must have this many virtqueues. */
if (nvqs > ioread8(&mvdev->desc->num_vq))
@@ -332,7 +332,7 @@ static int mic_find_vqs(struct virtio_device *vdev, unsigned nvqs,
* rings have been re-assigned.
*/
mic_send_intr(mvdev->mdev, mvdev->c2h_vdev_db);
- for (i = retry; i--;) {
+ for (retry = 100; retry--;) {
if (!ioread8(&dc->used_address_updated))
break;
msleep(100);
diff --git a/drivers/misc/mic/host/mic_virtio.c b/drivers/misc/mic/host/mic_virtio.c
index 5b8494b..945a3d0 100644
--- a/drivers/misc/mic/host/mic_virtio.c
+++ b/drivers/misc/mic/host/mic_virtio.c
@@ -378,7 +378,7 @@ int mic_virtio_config_change(struct mic_vdev *mvdev,
void __user *argp)
{
DECLARE_WAIT_QUEUE_HEAD_ONSTACK(wake);
- int ret = 0, retry = 100, i;
+ int ret = 0, retry, i;
struct mic_bootparam *bootparam = mvdev->mdev->dp;
s8 db = bootparam->h2c_config_db;
@@ -401,7 +401,7 @@ int mic_virtio_config_change(struct mic_vdev *mvdev,
mvdev->dc->config_change = MIC_VIRTIO_PARAM_CONFIG_CHANGED;
mvdev->mdev->ops->send_intr(mvdev->mdev, db);
- for (i = retry; i--;) {
+ for (retry = 100; retry--;) {
ret = wait_event_timeout(wake,
mvdev->dc->guest_ack, msecs_to_jiffies(100));
if (ret)
@@ -639,7 +639,7 @@ void mic_virtio_del_device(struct mic_vdev *mvdev)
struct mic_vdev *tmp_mvdev;
struct mic_device *mdev = mvdev->mdev;
DECLARE_WAIT_QUEUE_HEAD_ONSTACK(wake);
- int i, ret, retry = 100;
+ int i, ret, retry;
struct mic_vqconfig *vqconfig;
struct mic_bootparam *bootparam = mdev->dp;
s8 db;
@@ -652,16 +652,16 @@ void mic_virtio_del_device(struct mic_vdev *mvdev)
"Requesting hot remove id %d\n", mvdev->virtio_id);
mvdev->dc->config_change = MIC_VIRTIO_PARAM_DEV_REMOVE;
mdev->ops->send_intr(mdev, db);
- for (i = retry; i--;) {
+ for (retry = 100; retry--;) {
ret = wait_event_timeout(wake,
mvdev->dc->guest_ack, msecs_to_jiffies(100));
if (ret)
break;
}
dev_dbg(mdev->sdev->parent,
- "Device id %d config_change %d guest_ack %d\n",
+ "Device id %d config_change %d guest_ack %d retry %d\n",
mvdev->virtio_id, mvdev->dc->config_change,
- mvdev->dc->guest_ack);
+ mvdev->dc->guest_ack, retry);
mvdev->dc->config_change = 0;
mvdev->dc->guest_ack = 0;
skip_hot_remove:
--
1.8.2.3
^ permalink raw reply related [flat|nested] 9+ messages in thread* [PATCH char-misc-linus 3/5] misc: mic: Bug fix for sysfs poll usage.
2013-11-26 18:04 [PATCH char-misc-linus 0/5] misc: mic: Fixes for 3.13-rc2 Ashutosh Dixit
2013-11-26 18:08 ` [PATCH char-misc-linus 1/5] misc: mic: Change mic_notify(...) to return true Ashutosh Dixit
2013-11-26 18:11 ` [PATCH char-misc-linus 2/5] misc: mic: Minor bug fix in 'retry' loops Ashutosh Dixit
@ 2013-11-26 18:12 ` Ashutosh Dixit
2013-11-26 18:14 ` [PATCH char-misc-linus 4/5] misc: mic: Fix sparse warnings and other endianness issues Ashutosh Dixit
2013-11-26 18:15 ` [PATCH char-misc-linus 5/5] misc: mic: Fix user space namespace pollution from mic_common.h Ashutosh Dixit
4 siblings, 0 replies; 9+ messages in thread
From: Ashutosh Dixit @ 2013-11-26 18:12 UTC (permalink / raw)
To: Greg Kroah-Hartman, Arnd Bergmann, linux-kernel, virtualization
Cc: Dasaratharaman Chandramouli, Ashutosh Dixit, Sudeep Dutt,
Nikhil Rao, Siva Krishna Kumar Reddy Yerramreddy, Caz Yokoyama
MIC user space daemon poll's on sysfs changes. The documentation for
sysfs_poll(...) in fs/sysfs/file.c states that "Once poll/select
indicates that the value has changed, you need to close and re-open the
file, or seek to 0 and read again". This step was missed out earlier and
resulted in the daemon spinning continuously rather than getting blocked
in 'poll'. This bug was exposed by commit aea585ef8fa65163 introduced as
part of sysfs changes in 3.13-rc1. A seek to 0 has been introduced to
fix it.
Reviewed-by: Sudeep Dutt <sudeep.dutt@intel.com>
Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
---
Documentation/mic/mpssd/mpssd.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/Documentation/mic/mpssd/mpssd.c b/Documentation/mic/mpssd/mpssd.c
index 0c980ad..5c7fdda 100644
--- a/Documentation/mic/mpssd/mpssd.c
+++ b/Documentation/mic/mpssd/mpssd.c
@@ -1412,6 +1412,12 @@ mic_config(void *arg)
}
do {
+ ret = lseek(fd, 0, SEEK_SET);
+ if (ret < 0) {
+ mpsslog("%s: Failed to seek to file start '%s': %s\n",
+ mic->name, pathname, strerror(errno));
+ goto close_error1;
+ }
ret = read(fd, value, sizeof(value));
if (ret < 0) {
mpsslog("%s: Failed to read sysfs entry '%s': %s\n",
--
1.8.2.3
^ permalink raw reply related [flat|nested] 9+ messages in thread* [PATCH char-misc-linus 4/5] misc: mic: Fix sparse warnings and other endianness issues.
2013-11-26 18:04 [PATCH char-misc-linus 0/5] misc: mic: Fixes for 3.13-rc2 Ashutosh Dixit
` (2 preceding siblings ...)
2013-11-26 18:12 ` [PATCH char-misc-linus 3/5] misc: mic: Bug fix for sysfs poll usage Ashutosh Dixit
@ 2013-11-26 18:14 ` Ashutosh Dixit
2013-11-26 19:15 ` Greg Kroah-Hartman
2013-11-26 18:15 ` [PATCH char-misc-linus 5/5] misc: mic: Fix user space namespace pollution from mic_common.h Ashutosh Dixit
4 siblings, 1 reply; 9+ messages in thread
From: Ashutosh Dixit @ 2013-11-26 18:14 UTC (permalink / raw)
To: Greg Kroah-Hartman, Arnd Bergmann, linux-kernel, virtualization
Cc: Dasaratharaman Chandramouli, Ashutosh Dixit, Sudeep Dutt,
Nikhil Rao, Siva Krishna Kumar Reddy Yerramreddy, Caz Yokoyama,
Fengguang Wu
Endianness issues are now consistent as per the documentation in
host/mic_virtio.h. Note that the host can be both BE or LE whereas the
card is always LE.
Memory space sparse warnings are fixed for now by using __force. This is
sufficient for now since the driver depends on x86 but will need to be
revisited if we support other architectures which treat I/O memory
differently from system memory.
Reported-by: Fengguang Wu <fengguang.wu@intel.com>
Reviewed-by: Sudeep Dutt <sudeep.dutt@intel.com>
Cc: Nikhil Rao <nikhil.rao@intel.com>
Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
---
Documentation/mic/mpssd/mpssd.c | 8 ++++----
drivers/misc/mic/card/mic_virtio.c | 18 ++++++++++--------
drivers/misc/mic/host/mic_boot.c | 2 +-
drivers/misc/mic/host/mic_virtio.c | 16 ++++++++--------
drivers/misc/mic/host/mic_x100.c | 4 ++--
include/uapi/linux/mic_common.h | 14 +++++++-------
6 files changed, 32 insertions(+), 30 deletions(-)
diff --git a/Documentation/mic/mpssd/mpssd.c b/Documentation/mic/mpssd/mpssd.c
index 5c7fdda..befc2c3 100644
--- a/Documentation/mic/mpssd/mpssd.c
+++ b/Documentation/mic/mpssd/mpssd.c
@@ -445,8 +445,8 @@ init_vr(struct mic_info *mic, int fd, int type,
__func__, mic->name, vr0->va, vr0->info, vr_size,
vring_size(MIC_VRING_ENTRIES, MIC_VIRTIO_RING_ALIGN));
mpsslog("magic 0x%x expected 0x%x\n",
- vr0->info->magic, MIC_MAGIC + type);
- assert(vr0->info->magic == MIC_MAGIC + type);
+ le32toh(vr0->info->magic), MIC_MAGIC + type);
+ assert(le32toh(vr0->info->magic) == MIC_MAGIC + type);
if (vr1) {
vr1->va = (struct mic_vring *)
&va[MIC_DEVICE_PAGE_END + vr_size];
@@ -458,8 +458,8 @@ init_vr(struct mic_info *mic, int fd, int type,
__func__, mic->name, vr1->va, vr1->info, vr_size,
vring_size(MIC_VRING_ENTRIES, MIC_VIRTIO_RING_ALIGN));
mpsslog("magic 0x%x expected 0x%x\n",
- vr1->info->magic, MIC_MAGIC + type + 1);
- assert(vr1->info->magic == MIC_MAGIC + type + 1);
+ le32toh(vr1->info->magic), MIC_MAGIC + type + 1);
+ assert(le32toh(vr1->info->magic) == MIC_MAGIC + type + 1);
}
done:
return va;
diff --git a/drivers/misc/mic/card/mic_virtio.c b/drivers/misc/mic/card/mic_virtio.c
index 4dce912..c975c36 100644
--- a/drivers/misc/mic/card/mic_virtio.c
+++ b/drivers/misc/mic/card/mic_virtio.c
@@ -248,17 +248,17 @@ static struct virtqueue *mic_find_vq(struct virtio_device *vdev,
/* First assign the vring's allocated in host memory */
vqconfig = mic_vq_config(mvdev->desc) + index;
memcpy_fromio(&config, vqconfig, sizeof(config));
- _vr_size = vring_size(config.num, MIC_VIRTIO_RING_ALIGN);
+ _vr_size = vring_size(le16_to_cpu(config.num), MIC_VIRTIO_RING_ALIGN);
vr_size = PAGE_ALIGN(_vr_size + sizeof(struct _mic_vring_info));
- va = mic_card_map(mvdev->mdev, config.address, vr_size);
+ va = mic_card_map(mvdev->mdev, le64_to_cpu(config.address), vr_size);
if (!va)
return ERR_PTR(-ENOMEM);
mvdev->vr[index] = va;
memset_io(va, 0x0, _vr_size);
- vq = vring_new_virtqueue(index,
- config.num, MIC_VIRTIO_RING_ALIGN, vdev,
- false,
- va, mic_notify, callback, name);
+ vq = vring_new_virtqueue(index, le16_to_cpu(config.num),
+ MIC_VIRTIO_RING_ALIGN, vdev, false,
+ (void __force *)va, mic_notify, callback,
+ name);
if (!vq) {
err = -ENOMEM;
goto unmap;
@@ -273,7 +273,8 @@ static struct virtqueue *mic_find_vq(struct virtio_device *vdev,
/* Allocate and reassign used ring now */
mvdev->used_size[index] = PAGE_ALIGN(sizeof(__u16) * 3 +
- sizeof(struct vring_used_elem) * config.num);
+ sizeof(struct vring_used_elem) *
+ le16_to_cpu(config.num));
used = (void *)__get_free_pages(GFP_KERNEL | __GFP_ZERO,
get_order(mvdev->used_size[index]));
if (!used) {
@@ -540,7 +541,8 @@ static void mic_scan_devices(struct mic_driver *mdrv, bool remove)
continue;
/* device already exists */
- dev = device_find_child(mdrv->dev, d, mic_match_desc);
+ dev = device_find_child(mdrv->dev, (void __force *)d,
+ mic_match_desc);
if (dev) {
if (remove)
iowrite8(MIC_VIRTIO_PARAM_DEV_REMOVE,
diff --git a/drivers/misc/mic/host/mic_boot.c b/drivers/misc/mic/host/mic_boot.c
index 7558d91..b75c6b5 100644
--- a/drivers/misc/mic/host/mic_boot.c
+++ b/drivers/misc/mic/host/mic_boot.c
@@ -62,7 +62,7 @@ void mic_bootparam_init(struct mic_device *mdev)
{
struct mic_bootparam *bootparam = mdev->dp;
- bootparam->magic = MIC_MAGIC;
+ bootparam->magic = cpu_to_le32(MIC_MAGIC);
bootparam->c2h_shutdown_db = mdev->shutdown_db;
bootparam->h2c_shutdown_db = -1;
bootparam->h2c_config_db = -1;
diff --git a/drivers/misc/mic/host/mic_virtio.c b/drivers/misc/mic/host/mic_virtio.c
index 945a3d0..074e736 100644
--- a/drivers/misc/mic/host/mic_virtio.c
+++ b/drivers/misc/mic/host/mic_virtio.c
@@ -41,7 +41,7 @@ static int mic_virtio_copy_to_user(struct mic_vdev *mvdev,
* We are copying from IO below an should ideally use something
* like copy_to_user_fromio(..) if it existed.
*/
- if (copy_to_user(ubuf, dbuf, len)) {
+ if (copy_to_user(ubuf, (void __force *)dbuf, len)) {
err = -EFAULT;
dev_err(mic_dev(mvdev), "%s %d err %d\n",
__func__, __LINE__, err);
@@ -66,7 +66,7 @@ static int mic_virtio_copy_from_user(struct mic_vdev *mvdev,
* We are copying to IO below and should ideally use something
* like copy_from_user_toio(..) if it existed.
*/
- if (copy_from_user(dbuf, ubuf, len)) {
+ if (copy_from_user((void __force *)dbuf, ubuf, len)) {
err = -EFAULT;
dev_err(mic_dev(mvdev), "%s %d err %d\n",
__func__, __LINE__, err);
@@ -293,7 +293,7 @@ static void mic_virtio_init_post(struct mic_vdev *mvdev)
continue;
}
mvdev->mvr[i].vrh.vring.used =
- mvdev->mdev->aper.va +
+ (void __force *)mvdev->mdev->aper.va +
le64_to_cpu(vqconfig[i].used_address);
}
@@ -525,6 +525,7 @@ int mic_virtio_add_device(struct mic_vdev *mvdev,
char irqname[10];
struct mic_bootparam *bootparam = mdev->dp;
u16 num;
+ dma_addr_t vr_addr;
mutex_lock(&mdev->mic_mutex);
@@ -559,17 +560,16 @@ int mic_virtio_add_device(struct mic_vdev *mvdev,
}
vr->len = vr_size;
vr->info = vr->va + vring_size(num, MIC_VIRTIO_RING_ALIGN);
- vr->info->magic = MIC_MAGIC + mvdev->virtio_id + i;
- vqconfig[i].address = mic_map_single(mdev,
- vr->va, vr_size);
- if (mic_map_error(vqconfig[i].address)) {
+ vr->info->magic = cpu_to_le32(MIC_MAGIC + mvdev->virtio_id + i);
+ vr_addr = mic_map_single(mdev, vr->va, vr_size);
+ if (mic_map_error(vr_addr)) {
free_pages((unsigned long)vr->va, get_order(vr_size));
ret = -ENOMEM;
dev_err(mic_dev(mvdev), "%s %d err %d\n",
__func__, __LINE__, ret);
goto err;
}
- vqconfig[i].address = cpu_to_le64(vqconfig[i].address);
+ vqconfig[i].address = cpu_to_le64(vr_addr);
vring_init(&vr->vr, num, vr->va, MIC_VIRTIO_RING_ALIGN);
ret = vringh_init_kern(&mvr->vrh,
diff --git a/drivers/misc/mic/host/mic_x100.c b/drivers/misc/mic/host/mic_x100.c
index 81e9541..0dfa8a8 100644
--- a/drivers/misc/mic/host/mic_x100.c
+++ b/drivers/misc/mic/host/mic_x100.c
@@ -397,8 +397,8 @@ mic_x100_load_ramdisk(struct mic_device *mdev)
* so copy over the ramdisk @ 128M.
*/
memcpy_toio(mdev->aper.va + (mdev->bootaddr << 1), fw->data, fw->size);
- iowrite32(cpu_to_le32(mdev->bootaddr << 1), &bp->hdr.ramdisk_image);
- iowrite32(cpu_to_le32(fw->size), &bp->hdr.ramdisk_size);
+ iowrite32(mdev->bootaddr << 1, &bp->hdr.ramdisk_image);
+ iowrite32(fw->size, &bp->hdr.ramdisk_size);
release_firmware(fw);
error:
return rc;
diff --git a/include/uapi/linux/mic_common.h b/include/uapi/linux/mic_common.h
index 17e7d95..ca70a50 100644
--- a/include/uapi/linux/mic_common.h
+++ b/include/uapi/linux/mic_common.h
@@ -48,7 +48,7 @@ struct mic_device_desc {
__u8 feature_len;
__u8 config_len;
__u8 status;
- __u64 config[0];
+ __le64 config[0];
} __aligned(8);
/**
@@ -66,7 +66,7 @@ struct mic_device_desc {
* @h2c_vdev_db: The doorbell number to be used by host. Set by guest.
*/
struct mic_device_ctrl {
- __u64 vdev;
+ __le64 vdev;
__u8 config_change;
__u8 vdev_reset;
__u8 guest_ack;
@@ -87,7 +87,7 @@ struct mic_device_ctrl {
* @shutdown_card: Set to 1 by the host when a card shutdown is initiated
*/
struct mic_bootparam {
- __u32 magic;
+ __le32 magic;
__s8 c2h_shutdown_db;
__s8 h2c_shutdown_db;
__s8 h2c_config_db;
@@ -116,9 +116,9 @@ struct mic_device_page {
* @num: The number of entries in the virtio_ring
*/
struct mic_vqconfig {
- __u64 address;
- __u64 used_address;
- __u16 num;
+ __le64 address;
+ __le64 used_address;
+ __le16 num;
} __aligned(8);
/*
@@ -154,7 +154,7 @@ struct mic_vqconfig {
*/
struct _mic_vring_info {
__u16 avail_idx;
- int magic;
+ __le32 magic;
};
/**
--
1.8.2.3
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH char-misc-linus 4/5] misc: mic: Fix sparse warnings and other endianness issues.
2013-11-26 18:14 ` [PATCH char-misc-linus 4/5] misc: mic: Fix sparse warnings and other endianness issues Ashutosh Dixit
@ 2013-11-26 19:15 ` Greg Kroah-Hartman
2013-11-27 17:12 ` Ashutosh Dixit
0 siblings, 1 reply; 9+ messages in thread
From: Greg Kroah-Hartman @ 2013-11-26 19:15 UTC (permalink / raw)
To: Ashutosh Dixit
Cc: Arnd Bergmann, linux-kernel, virtualization,
Dasaratharaman Chandramouli, Sudeep Dutt, Nikhil Rao,
Siva Krishna Kumar Reddy Yerramreddy, Caz Yokoyama, Fengguang Wu
On Tue, Nov 26, 2013 at 10:14:21AM -0800, Ashutosh Dixit wrote:
> Endianness issues are now consistent as per the documentation in
> host/mic_virtio.h. Note that the host can be both BE or LE whereas the
> card is always LE.
>
> Memory space sparse warnings are fixed for now by using __force. This is
> sufficient for now since the driver depends on x86 but will need to be
> revisited if we support other architectures which treat I/O memory
> differently from system memory.
There's no need for this for 3.13-final, right? No bug fixes are here
that I can tell.
And don't use __force, really, can't you fix this some other way?
> diff --git a/drivers/misc/mic/card/mic_virtio.c b/drivers/misc/mic/card/mic_virtio.c
> index 4dce912..c975c36 100644
> --- a/drivers/misc/mic/card/mic_virtio.c
> +++ b/drivers/misc/mic/card/mic_virtio.c
> @@ -248,17 +248,17 @@ static struct virtqueue *mic_find_vq(struct virtio_device *vdev,
> /* First assign the vring's allocated in host memory */
> vqconfig = mic_vq_config(mvdev->desc) + index;
> memcpy_fromio(&config, vqconfig, sizeof(config));
> - _vr_size = vring_size(config.num, MIC_VIRTIO_RING_ALIGN);
> + _vr_size = vring_size(le16_to_cpu(config.num), MIC_VIRTIO_RING_ALIGN);
> vr_size = PAGE_ALIGN(_vr_size + sizeof(struct _mic_vring_info));
> - va = mic_card_map(mvdev->mdev, config.address, vr_size);
> + va = mic_card_map(mvdev->mdev, le64_to_cpu(config.address), vr_size);
> if (!va)
> return ERR_PTR(-ENOMEM);
> mvdev->vr[index] = va;
> memset_io(va, 0x0, _vr_size);
> - vq = vring_new_virtqueue(index,
> - config.num, MIC_VIRTIO_RING_ALIGN, vdev,
> - false,
> - va, mic_notify, callback, name);
> + vq = vring_new_virtqueue(index, le16_to_cpu(config.num),
> + MIC_VIRTIO_RING_ALIGN, vdev, false,
> + (void __force *)va, mic_notify, callback,
> + name);
Why __force a void * here? That feels wrong.
Can you split the endian fixes up from the user pointer fixes to make it
easier to review/apply?
thanks,
greg k-h
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH char-misc-linus 4/5] misc: mic: Fix sparse warnings and other endianness issues.
2013-11-26 19:15 ` Greg Kroah-Hartman
@ 2013-11-27 17:12 ` Ashutosh Dixit
0 siblings, 0 replies; 9+ messages in thread
From: Ashutosh Dixit @ 2013-11-27 17:12 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Arnd Bergmann, linux-kernel@vger.kernel.org,
virtualization@lists.linux-foundation.org,
Chandramouli, Dasaratharaman, Dutt, Sudeep, Rao, Nikhil,
Yerramreddy, Siva Krishna Kumar Reddy, Yokoyama, Caz,
Wu, Fengguang
On Tue, Nov 26 2013 at 11:15:25 AM, Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
> On Tue, Nov 26, 2013 at 10:14:21AM -0800, Ashutosh Dixit wrote:
>> Endianness issues are now consistent as per the documentation in
>> host/mic_virtio.h. Note that the host can be both BE or LE whereas the
>> card is always LE.
>>
>> Memory space sparse warnings are fixed for now by using __force. This is
>> sufficient for now since the driver depends on x86 but will need to be
>> revisited if we support other architectures which treat I/O memory
>> differently from system memory.
>
> Can you split the endian fixes up from the user pointer fixes to make it
> easier to review/apply?
We've just submitted v3 where we now have separate patches (5 and 6) for
endianness and sparse memory access fixes.
> There's no need for this for 3.13-final, right? No bug fixes are here
> that I can tell.
That's correct. I've reordered the patches so that v3 patches 1-4
contain the real bug fixes. We'd like to apply patches 5 and 6 for
3.13-final too but if it is not possible we can drop one or both of them
for now for further review.
> And don't use __force, really, can't you fix this some other way?
As stated in the description for patch 6, because of the dependence of
the MIC drivers on the virtio infrastructure there is no simple way to
fix these memory space issues at present. Since the sparse warnings do
not represent real issues the warnings are being suppressed using
__force.
>> + vq = vring_new_virtqueue(index, le16_to_cpu(config.num),
>> + MIC_VIRTIO_RING_ALIGN, vdev, false,
>> + (void __force *)va, mic_notify, callback,
>> + name);
>
> Why __force a void * here? That feels wrong.
In all instances in patch 6 __force is used to strip the __iomem
attribute (e.g. va is declared as 'void __iomem *va;').
thanks,
ashutosh
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH char-misc-linus 5/5] misc: mic: Fix user space namespace pollution from mic_common.h.
2013-11-26 18:04 [PATCH char-misc-linus 0/5] misc: mic: Fixes for 3.13-rc2 Ashutosh Dixit
` (3 preceding siblings ...)
2013-11-26 18:14 ` [PATCH char-misc-linus 4/5] misc: mic: Fix sparse warnings and other endianness issues Ashutosh Dixit
@ 2013-11-26 18:15 ` Ashutosh Dixit
2013-11-26 18:12 ` H. Peter Anvin
4 siblings, 1 reply; 9+ messages in thread
From: Ashutosh Dixit @ 2013-11-26 18:15 UTC (permalink / raw)
To: Greg Kroah-Hartman, Arnd Bergmann, linux-kernel, virtualization
Cc: Dasaratharaman Chandramouli, Ashutosh Dixit, Sudeep Dutt,
Nikhil Rao, Siva Krishna Kumar Reddy Yerramreddy, Caz Yokoyama,
H Peter Anvin, Peter P Waskiewicz Jr
Avoid declaring ALIGN() and __aligned() in
include/uapi/linux/mic_common.h since they pollute user space
namespace. Also, mic_aligned_size() can be simply replaced simply by
sizeof() since all structures where mic_aligned_size() is used are
declared using __attribute__ ((aligned(8)));
--
>From mail from H Peter Anvin about this:
On Fri, Nov 08, 2013 H Peter Anvin <h.peter.anvin@intel.com> wrote:
Subject: Namespace pollution in mic_common.h
This puts two macros, ALIGN() and __aligned(), into arbitrary user space
namespace. This really isn't safe or acceptable, especially since those
symbols are highly generic.
...
When these structures are forced-aligned, they will in fact have padding
automatically added by the compiler to an 8-byte boundary anyway, so
mic_aligned_size() does nothing.
...
Reported-by: H Peter Anvin <h.peter.anvin@intel.com>
Reviewed-by: Sudeep Dutt <sudeep.dutt@intel.com>
Signed-off-by: Nikhil Rao <nikhil.rao@intel.com>
Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
---
Documentation/mic/mpssd/mpssd.c | 4 ++--
drivers/misc/mic/card/mic_virtio.c | 4 ++--
drivers/misc/mic/card/mic_virtio.h | 7 +++----
drivers/misc/mic/host/mic_virtio.c | 2 +-
include/uapi/linux/mic_common.h | 26 +++++++++-----------------
5 files changed, 17 insertions(+), 26 deletions(-)
diff --git a/Documentation/mic/mpssd/mpssd.c b/Documentation/mic/mpssd/mpssd.c
index befc2c3..4d17487 100644
--- a/Documentation/mic/mpssd/mpssd.c
+++ b/Documentation/mic/mpssd/mpssd.c
@@ -313,7 +313,7 @@ static struct mic_device_desc *get_device_desc(struct mic_info *mic, int type)
int i;
void *dp = get_dp(mic, type);
- for (i = mic_aligned_size(struct mic_bootparam); i < PAGE_SIZE;
+ for (i = sizeof(struct mic_bootparam); i < PAGE_SIZE;
i += mic_total_desc_size(d)) {
d = dp + i;
@@ -520,7 +520,7 @@ static void *
virtio_net(void *arg)
{
static __u8 vnet_hdr[2][sizeof(struct virtio_net_hdr)];
- static __u8 vnet_buf[2][MAX_NET_PKT_SIZE] __aligned(64);
+ static __u8 vnet_buf[2][MAX_NET_PKT_SIZE] __attribute__ ((aligned(64)));
struct iovec vnet_iov[2][2] = {
{ { .iov_base = vnet_hdr[0], .iov_len = sizeof(vnet_hdr[0]) },
{ .iov_base = vnet_buf[0], .iov_len = sizeof(vnet_buf[0]) } },
diff --git a/drivers/misc/mic/card/mic_virtio.c b/drivers/misc/mic/card/mic_virtio.c
index c975c36..653799b 100644
--- a/drivers/misc/mic/card/mic_virtio.c
+++ b/drivers/misc/mic/card/mic_virtio.c
@@ -521,8 +521,8 @@ static void mic_scan_devices(struct mic_driver *mdrv, bool remove)
struct device *dev;
int ret;
- for (i = mic_aligned_size(struct mic_bootparam);
- i < MIC_DP_SIZE; i += mic_total_desc_size(d)) {
+ for (i = sizeof(struct mic_bootparam); i < MIC_DP_SIZE;
+ i += mic_total_desc_size(d)) {
d = mdrv->dp + i;
dc = (void __iomem *)d + mic_aligned_desc_size(d);
/*
diff --git a/drivers/misc/mic/card/mic_virtio.h b/drivers/misc/mic/card/mic_virtio.h
index 2c5c22c..d0407ba 100644
--- a/drivers/misc/mic/card/mic_virtio.h
+++ b/drivers/misc/mic/card/mic_virtio.h
@@ -42,8 +42,8 @@
static inline unsigned mic_desc_size(struct mic_device_desc __iomem *desc)
{
- return mic_aligned_size(*desc)
- + ioread8(&desc->num_vq) * mic_aligned_size(struct mic_vqconfig)
+ return sizeof(*desc)
+ + ioread8(&desc->num_vq) * sizeof(struct mic_vqconfig)
+ ioread8(&desc->feature_len) * 2
+ ioread8(&desc->config_len);
}
@@ -67,8 +67,7 @@ mic_vq_configspace(struct mic_device_desc __iomem *desc)
}
static inline unsigned mic_total_desc_size(struct mic_device_desc __iomem *desc)
{
- return mic_aligned_desc_size(desc) +
- mic_aligned_size(struct mic_device_ctrl);
+ return mic_aligned_desc_size(desc) + sizeof(struct mic_device_ctrl);
}
int mic_devices_init(struct mic_driver *mdrv);
diff --git a/drivers/misc/mic/host/mic_virtio.c b/drivers/misc/mic/host/mic_virtio.c
index 074e736..e04bb4f 100644
--- a/drivers/misc/mic/host/mic_virtio.c
+++ b/drivers/misc/mic/host/mic_virtio.c
@@ -467,7 +467,7 @@ static int mic_copy_dp_entry(struct mic_vdev *mvdev,
}
/* Find the first free device page entry */
- for (i = mic_aligned_size(struct mic_bootparam);
+ for (i = sizeof(struct mic_bootparam);
i < MIC_DP_SIZE - mic_total_desc_size(dd_config);
i += mic_total_desc_size(devp)) {
devp = mdev->dp + i;
diff --git a/include/uapi/linux/mic_common.h b/include/uapi/linux/mic_common.h
index ca70a50..068f55d 100644
--- a/include/uapi/linux/mic_common.h
+++ b/include/uapi/linux/mic_common.h
@@ -23,12 +23,7 @@
#include <linux/virtio_ring.h>
-#ifndef __KERNEL__
-#define ALIGN(a, x) (((a) + (x) - 1) & ~((x) - 1))
-#define __aligned(x) __attribute__ ((aligned(x)))
-#endif
-
-#define mic_aligned_size(x) ALIGN(sizeof(x), 8)
+#define MIC_ALIGN(a, x) (((a) + (x) - 1) & ~((x) - 1))
/**
* struct mic_device_desc: Virtio device information shared between the
@@ -49,7 +44,7 @@ struct mic_device_desc {
__u8 config_len;
__u8 status;
__le64 config[0];
-} __aligned(8);
+} __attribute__ ((aligned(8)));
/**
* struct mic_device_ctrl: Per virtio device information in the device page
@@ -74,7 +69,7 @@ struct mic_device_ctrl {
__u8 used_address_updated;
__s8 c2h_vdev_db;
__s8 h2c_vdev_db;
-} __aligned(8);
+} __attribute__ ((aligned(8)));
/**
* struct mic_bootparam: Virtio device independent information in device page
@@ -93,7 +88,7 @@ struct mic_bootparam {
__s8 h2c_config_db;
__u8 shutdown_status;
__u8 shutdown_card;
-} __aligned(8);
+} __attribute__ ((aligned(8)));
/**
* struct mic_device_page: High level representation of the device page
@@ -119,7 +114,7 @@ struct mic_vqconfig {
__le64 address;
__le64 used_address;
__le16 num;
-} __aligned(8);
+} __attribute__ ((aligned(8)));
/*
* The alignment to use between consumer and producer parts of vring.
@@ -173,15 +168,13 @@ struct mic_vring {
int len;
};
-#define mic_aligned_desc_size(d) ALIGN(mic_desc_size(d), 8)
+#define mic_aligned_desc_size(d) MIC_ALIGN(mic_desc_size(d), 8)
#ifndef INTEL_MIC_CARD
static inline unsigned mic_desc_size(const struct mic_device_desc *desc)
{
- return mic_aligned_size(*desc)
- + desc->num_vq * mic_aligned_size(struct mic_vqconfig)
- + desc->feature_len * 2
- + desc->config_len;
+ return sizeof(*desc) + desc->num_vq * sizeof(struct mic_vqconfig)
+ + desc->feature_len * 2 + desc->config_len;
}
static inline struct mic_vqconfig *
@@ -201,8 +194,7 @@ static inline __u8 *mic_vq_configspace(const struct mic_device_desc *desc)
}
static inline unsigned mic_total_desc_size(struct mic_device_desc *desc)
{
- return mic_aligned_desc_size(desc) +
- mic_aligned_size(struct mic_device_ctrl);
+ return mic_aligned_desc_size(desc) + sizeof(struct mic_device_ctrl);
}
#endif
--
1.8.2.3
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH char-misc-linus 5/5] misc: mic: Fix user space namespace pollution from mic_common.h.
2013-11-26 18:15 ` [PATCH char-misc-linus 5/5] misc: mic: Fix user space namespace pollution from mic_common.h Ashutosh Dixit
@ 2013-11-26 18:12 ` H. Peter Anvin
0 siblings, 0 replies; 9+ messages in thread
From: H. Peter Anvin @ 2013-11-26 18:12 UTC (permalink / raw)
To: Ashutosh Dixit, Greg Kroah-Hartman, Arnd Bergmann, linux-kernel,
virtualization
Cc: Dasaratharaman Chandramouli, Sudeep Dutt, Nikhil Rao,
Siva Krishna Kumar Reddy Yerramreddy, Caz Yokoyama,
Peter P Waskiewicz Jr
On 11/26/2013 10:15 AM, Ashutosh Dixit wrote:
> Avoid declaring ALIGN() and __aligned() in
> include/uapi/linux/mic_common.h since they pollute user space
> namespace. Also, mic_aligned_size() can be simply replaced simply by
> sizeof() since all structures where mic_aligned_size() is used are
> declared using __attribute__ ((aligned(8)));
>
> +#define MIC_ALIGN(a, x) (((a) + (x) - 1) & ~((x) - 1))
>
This is still user namespace. I would suggest __mic_align() or
_MIC_ALIGN() instead, unless you're actively expecting user code to make
use of this macro (in which case it should be made more robust.)
-hpa
^ permalink raw reply [flat|nested] 9+ messages in thread