* [PATCH] sdhci-esdhc-imx: Correct two register accesses
From: Aaron Brice @ 2016-10-06 22:54 UTC (permalink / raw)
To: linux-arm-kernel
- The DMA error interrupt bit is in a different position as
compared to the sdhci standard. This is accounted for in
many cases, but not handled in the case of clearing the
INT_STATUS register by writing a 1 to that location.
- The HOST_CONTROL register is very different as compared to
the sdhci standard. This is accounted for in the write
case, but not when read back out (which it is in the sdhci
code).
Signed-off-by: Dave Russell <david.russell@datasoft.com>
Signed-off-by: Aaron Brice <aaron.brice@datasoft.com>
---
drivers/mmc/host/sdhci-esdhc-imx.c | 23 ++++++++++++++++++++++-
1 file changed, 22 insertions(+), 1 deletion(-)
diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
index 1f54fd8..d61ef16 100644
--- a/drivers/mmc/host/sdhci-esdhc-imx.c
+++ b/drivers/mmc/host/sdhci-esdhc-imx.c
@@ -346,7 +346,8 @@ static void esdhc_writel_le(struct sdhci_host *host, u32 val, int reg)
struct pltfm_imx_data *imx_data = sdhci_pltfm_priv(pltfm_host);
u32 data;
- if (unlikely(reg == SDHCI_INT_ENABLE || reg == SDHCI_SIGNAL_ENABLE)) {
+ if (unlikely(reg == SDHCI_INT_ENABLE || reg == SDHCI_SIGNAL_ENABLE ||
+ reg == SDHCI_INT_STATUS)) {
if ((val & SDHCI_INT_CARD_INT) && !esdhc_is_usdhc(imx_data)) {
/*
* Clear and then set D3CD bit to avoid missing the
@@ -555,6 +556,25 @@ static void esdhc_writew_le(struct sdhci_host *host, u16 val, int reg)
esdhc_clrset_le(host, 0xffff, val, reg);
}
+static u8 esdhc_readb_le(struct sdhci_host *host, int reg)
+{
+ u8 ret;
+ u32 long_val;
+
+ switch (reg) {
+ case SDHCI_HOST_CONTROL:
+ long_val = readl(host->ioaddr + reg);
+
+ ret = long_val & SDHCI_CTRL_LED;
+ ret |= (long_val >> 5) & SDHCI_CTRL_DMA_MASK;
+ ret |= (long_val & ESDHC_CTRL_4BITBUS);
+ ret |= (long_val & ESDHC_CTRL_8BITBUS) << 3;
+ return ret;
+ }
+
+ return readb(host->ioaddr + reg);
+}
+
static void esdhc_writeb_le(struct sdhci_host *host, u8 val, int reg)
{
struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
@@ -947,6 +967,7 @@ static void esdhc_set_timeout(struct sdhci_host *host, struct mmc_command *cmd)
static struct sdhci_ops sdhci_esdhc_ops = {
.read_l = esdhc_readl_le,
.read_w = esdhc_readw_le,
+ .read_b = esdhc_readb_le,
.write_l = esdhc_writel_le,
.write_w = esdhc_writew_le,
.write_b = esdhc_writeb_le,
--
2.7.4
^ permalink raw reply related
* [PATCH] Revert "debugfs: ->d_parent is never NULL or negative"
From: Al Viro @ 2016-10-06 22:41 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161006223729.GO19539@ZenIV.linux.org.uk>
On Thu, Oct 06, 2016 at 11:37:29PM +0100, Al Viro wrote:
> If you ever get NULL in ->d_parent of struct dentry instance, you are
> practically certain to have a dangling pointer to memory that used to
> contain a struct dentry at some point but got freed and reused since then.
... which is what happens in your case, apparently. ->stats is still
pointing to a dentry that had just been freed and its memory reused.
^ permalink raw reply
* [PATCH] Revert "debugfs: ->d_parent is never NULL or negative"
From: Al Viro @ 2016-10-06 22:37 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161006220050.GN19539@ZenIV.linux.org.uk>
On Thu, Oct 06, 2016 at 11:00:51PM +0100, Al Viro wrote:
> On Thu, Oct 06, 2016 at 05:41:34PM -0400, Sinan Kaya wrote:
> > On 10/6/2016 5:37 PM, Al Viro wrote:
> > > On Thu, Oct 06, 2016 at 05:30:29PM -0400, Sinan Kaya wrote:
> > >> This reverts commit acc29fb8f792 ("debugfs: ->d_parent is never NULL or
> > >> negative") as it breaks the debugfs_remove_recursive API as show in the
> > >> callstack below.
> > >
> > > NAK. Fix your code, don't break global asserts.
> > >
> >
> > I can fix the code if you tell me what the problem is:
>
> Getting dentries with NULL ->d_parent should never, ever happen. Find the
> place where such a beast appears and you've got your problem.
>
> The same goes for negative dentries with children. Again, if your code
> triggers such a situation, find where it does so and you've found a bug.
> More than one, at that.
Note that there are exactly 4 places where ->d_parent of some struct
dentry is modified, all in fs/dcache.c.
1) __d_alloc() sets ->d_parent of new instance pointing to the instance
itself and does so before anyone else could observe that dentry. That's
the only allocator of struct dentry - all of them start their life when
returned by it. With non-NULL value of ->d_parent.
2) d_alloc() sets it to given (non-NULL) parent. Note that it has
already dereferenced that parent (spin_lock(&parent->d_lock) a couple of
lines prior to that), so it would've oopsed before it reached that assignment
if it was passed NULL as parent.
3) d_alloc_cursor() - ditto, only there it had been an access to parent->d_sb.
4)?__d_move() does
dentry->d_parent = target->d_parent;
target->d_parent = target;
in one case and
swap(dentry->d_parent, target->d_parent);
in another. The values had either already been in ->d_parent of another
instance prior to that or are guaranteed to be non-NULL since we'd just
survived dereferencing them.
If you ever get NULL in ->d_parent of struct dentry instance, you are
practically certain to have a dangling pointer to memory that used to
contain a struct dentry at some point but got freed and reused since then.
Any such case is a bug, and this check only papers over that bug - after all,
we might have very well reused it for anything whatsoever, with arbitrary
values ending up in it.
As for the negatives...
* if ->d_parent points to something other than dentry itself,
it contributes to ->d_count of parent
* positive dentry can only be turned into negative if the
caller of d_delete() is holding the only reference to it
* any code setting ->d_parent to another dentry does so only when
the parent to be is known to be positive at the moment.
So if you get a dentry with negative parent passed to it, the only way
it could happen (aside of outright memory corruption) is that dentry
you are passing has ->d_parent pointing to *itself* (and had never been
made positive). If that can legitimately happen, the proper test is
IS_ROOT(dentry) && !dentry->d_inode, and I strongly suspect that the
second part is irrelevant. But I would really like to see what leads
to that - I don't see any way for debugfs_create_{file,dir}() et.al. to
return a detached (or negative, for that matter) dentry.
are
^ permalink raw reply
* [PATCH] dmaengine: qcom_hidma: remove useless debugfs file removal
From: Sinan Kaya @ 2016-10-06 22:21 UTC (permalink / raw)
To: linux-arm-kernel
Since 'commit acc29fb8f792 ("debugfs: ->d_parent is never NULL or
negative")', HIDMA object removal is no longer working. This is due to
redundant debugfs remove call in hidma_debug_uninit.
debugfs_remove_recursive(dmadev->debugfs);
debugfs_remove_recursive(dmadev->stats);
The first remove is for the directory. Second remove is for the file under
the directory. The directory remove makes file remove invalid.
Unable to handle kernel NULL pointer dereference at virtual address
[<ffff00000889f480>] down_write+0x18/0x68
[<ffff00000831c220>] debugfs_remove_recursive+0x50/0x1c0
[<ffff00000848e0a8>] hidma_debug_uninit+0x20/0x30
[<ffff00000848c5d8>] hidma_remove+0x48/0x98
[<ffff000008511b6c>] platform_drv_remove+0x24/0x68
[<ffff00000850fac8>] __device_release_driver+0x80/0x118
[<ffff00000850fb84>] device_release_driver+0x24/0x38
[<ffff00000850e928>] unbind_store+0xe8/0x110
[<ffff00000850dd30>] drv_attr_store+0x20/0x30
[<ffff000008253a48>] sysfs_kf_write+0x48/0x58
[<ffff000008252dd8>] kernfs_fop_write+0xb0/0x1d8
[<ffff0000081dab3c>] __vfs_write+0x1c/0x110
[<ffff0000081db940>] vfs_write+0xa0/0x1b8
[<ffff0000081dcd34>] SyS_write+0x44/0xa0
[<ffff000008082ef0>] el0_svc_naked+0x24/0x28
Removing the second line.
Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
---
drivers/dma/qcom/hidma_dbg.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/dma/qcom/hidma_dbg.c b/drivers/dma/qcom/hidma_dbg.c
index 87db285..3bdcb80 100644
--- a/drivers/dma/qcom/hidma_dbg.c
+++ b/drivers/dma/qcom/hidma_dbg.c
@@ -165,7 +165,6 @@ static int hidma_dma_info_open(struct inode *inode, struct file *file)
void hidma_debug_uninit(struct hidma_dev *dmadev)
{
debugfs_remove_recursive(dmadev->debugfs);
- debugfs_remove_recursive(dmadev->stats);
}
int hidma_debug_init(struct hidma_dev *dmadev)
--
1.9.1
^ permalink raw reply related
* [PATCH] tty: serial: fsl_lpuart: Fix Tx DMA edge case
From: Aaron Brice @ 2016-10-06 22:13 UTC (permalink / raw)
To: linux-arm-kernel
In the case where head == 0 on the circular buffer, there should be one
DMA buffer, not two. The second zero-length buffer would break the
lpuart driver, transfer would never complete.
Signed-off-by: Aaron Brice <aaron.brice@datasoft.com>
---
drivers/tty/serial/fsl_lpuart.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/tty/serial/fsl_lpuart.c b/drivers/tty/serial/fsl_lpuart.c
index de9d510..76103f2 100644
--- a/drivers/tty/serial/fsl_lpuart.c
+++ b/drivers/tty/serial/fsl_lpuart.c
@@ -328,7 +328,7 @@ static void lpuart_dma_tx(struct lpuart_port *sport)
sport->dma_tx_bytes = uart_circ_chars_pending(xmit);
- if (xmit->tail < xmit->head) {
+ if (xmit->tail < xmit->head || xmit->head == 0) {
sport->dma_tx_nents = 1;
sg_init_one(sgl, xmit->buf + xmit->tail, sport->dma_tx_bytes);
} else {
@@ -359,7 +359,6 @@ static void lpuart_dma_tx(struct lpuart_port *sport)
sport->dma_tx_in_progress = true;
sport->dma_tx_cookie = dmaengine_submit(sport->dma_tx_desc);
dma_async_issue_pending(sport->dma_tx_chan);
-
}
static void lpuart_dma_tx_complete(void *arg)
--
2.7.4
^ permalink raw reply related
* [PATCH] Revert "debugfs: ->d_parent is never NULL or negative"
From: Sinan Kaya @ 2016-10-06 22:11 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <ead77f2f-ec0f-f31a-428c-9671faaa30f9@codeaurora.org>
On 10/6/2016 5:56 PM, Sinan Kaya wrote:
> The fact that directory is removed might be leaving the stats in limbo.
>
> Let me test without this line.
This did the trick. Thanks for the heads up. The second line was a left over
from the code review. I was removing files piece by piece at the beginning.
Then, somebody said why don't you use recursive. While changing it to recursive,
I forgot to remove the second line.
--
Sinan Kaya
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
^ permalink raw reply
* [PATCH] Revert "debugfs: ->d_parent is never NULL or negative"
From: Al Viro @ 2016-10-06 22:00 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <f6ba2bf7-4f0b-e874-d642-6620d5df2212@codeaurora.org>
On Thu, Oct 06, 2016 at 05:41:34PM -0400, Sinan Kaya wrote:
> On 10/6/2016 5:37 PM, Al Viro wrote:
> > On Thu, Oct 06, 2016 at 05:30:29PM -0400, Sinan Kaya wrote:
> >> This reverts commit acc29fb8f792 ("debugfs: ->d_parent is never NULL or
> >> negative") as it breaks the debugfs_remove_recursive API as show in the
> >> callstack below.
> >
> > NAK. Fix your code, don't break global asserts.
> >
>
> I can fix the code if you tell me what the problem is:
Getting dentries with NULL ->d_parent should never, ever happen. Find the
place where such a beast appears and you've got your problem.
The same goes for negative dentries with children. Again, if your code
triggers such a situation, find where it does so and you've found a bug.
More than one, at that.
^ permalink raw reply
* [PATCH] Revert "debugfs: ->d_parent is never NULL or negative"
From: Sinan Kaya @ 2016-10-06 21:56 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <f6ba2bf7-4f0b-e874-d642-6620d5df2212@codeaurora.org>
On 10/6/2016 5:41 PM, Sinan Kaya wrote:
> On 10/6/2016 5:37 PM, Al Viro wrote:
>> On Thu, Oct 06, 2016 at 05:30:29PM -0400, Sinan Kaya wrote:
>>> This reverts commit acc29fb8f792 ("debugfs: ->d_parent is never NULL or
>>> negative") as it breaks the debugfs_remove_recursive API as show in the
>>> callstack below.
>>
>> NAK. Fix your code, don't break global asserts.
>>
>
> I can fix the code if you tell me what the problem is:
>
> http://lxr.free-electrons.com/ident?i=hidma_debug_uninit
>
> http://lxr.free-electrons.com/ident?i=hidma_debug_init
>
This is the directory structure:
/sys/kernel/debug/QCOM8061:00 # ls
chan0 stats
/sys/kernel/debug/QCOM8061:00 # cd chan0
/sys/kernel/debug/QCOM8061:00/chan0 # ls
stats
This is the QCOM8061:00 directory
debugfs_remove_recursive(dmadev->debugfs);
This is the stats file under the directory
debugfs_remove_recursive(dmadev->stats);
The fact that directory is removed might be leaving the stats in limbo.
Let me test without this line.
> The code didn't change between these commits.
>
> git log --oneline fs/debugfs/inode.c
>
> [doesn't work]
> 23f8a05 Merge branches 'work.misc', 'work.iget', 'work.const-qstr', 'work.splice_read' and 'current_time', remote-tracking branches 'ovl/misc' and 'ovl/
> c2050a4 fs: Replace current_fs_time() with current_time()
> e0e0be8 libfs: support RENAME_NOREPLACE in simple_rename()
> acc29fb debugfs: ->d_parent is never NULL or negative
> 5614e77 Merge 4.6-rc4 into driver-core-next
> 87243de debugfs: Make automount point inodes permanently empty
> c646880 debugfs: add support for self-protecting attribute file fops
>
> [works]
> dde78b1 Revert "debugfs: ->d_parent is never NULL or negative"
> 23f8a05 Merge branches 'work.misc', 'work.iget', 'work.const-qstr', 'work.splice_read' and 'current_time', remote-tracking branches 'ovl/misc' and 'ovl/
> c2050a4 fs: Replace current_fs_time() with current_time()
> e0e0be8 libfs: support RENAME_NOREPLACE in simple_rename()
> acc29fb debugfs: ->d_parent is never NULL or negative
> 5614e77 Merge 4.6-rc4 into driver-core-next
>
>
>
>
--
Sinan Kaya
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
^ permalink raw reply
* [PATCH] Revert "debugfs: ->d_parent is never NULL or negative"
From: Sinan Kaya @ 2016-10-06 21:41 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161006213734.GM19539@ZenIV.linux.org.uk>
On 10/6/2016 5:37 PM, Al Viro wrote:
> On Thu, Oct 06, 2016 at 05:30:29PM -0400, Sinan Kaya wrote:
>> This reverts commit acc29fb8f792 ("debugfs: ->d_parent is never NULL or
>> negative") as it breaks the debugfs_remove_recursive API as show in the
>> callstack below.
>
> NAK. Fix your code, don't break global asserts.
>
I can fix the code if you tell me what the problem is:
http://lxr.free-electrons.com/ident?i=hidma_debug_uninit
http://lxr.free-electrons.com/ident?i=hidma_debug_init
The code didn't change between these commits.
git log --oneline fs/debugfs/inode.c
[doesn't work]
23f8a05 Merge branches 'work.misc', 'work.iget', 'work.const-qstr', 'work.splice_read' and 'current_time', remote-tracking branches 'ovl/misc' and 'ovl/
c2050a4 fs: Replace current_fs_time() with current_time()
e0e0be8 libfs: support RENAME_NOREPLACE in simple_rename()
acc29fb debugfs: ->d_parent is never NULL or negative
5614e77 Merge 4.6-rc4 into driver-core-next
87243de debugfs: Make automount point inodes permanently empty
c646880 debugfs: add support for self-protecting attribute file fops
[works]
dde78b1 Revert "debugfs: ->d_parent is never NULL or negative"
23f8a05 Merge branches 'work.misc', 'work.iget', 'work.const-qstr', 'work.splice_read' and 'current_time', remote-tracking branches 'ovl/misc' and 'ovl/
c2050a4 fs: Replace current_fs_time() with current_time()
e0e0be8 libfs: support RENAME_NOREPLACE in simple_rename()
acc29fb debugfs: ->d_parent is never NULL or negative
5614e77 Merge 4.6-rc4 into driver-core-next
--
Sinan Kaya
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
^ permalink raw reply
* [PATCH] Revert "debugfs: ->d_parent is never NULL or negative"
From: Al Viro @ 2016-10-06 21:37 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1475789429-11414-1-git-send-email-okaya@codeaurora.org>
On Thu, Oct 06, 2016 at 05:30:29PM -0400, Sinan Kaya wrote:
> This reverts commit acc29fb8f792 ("debugfs: ->d_parent is never NULL or
> negative") as it breaks the debugfs_remove_recursive API as show in the
> callstack below.
NAK. Fix your code, don't break global asserts.
^ permalink raw reply
* [PATCH net-next 2/2] arm64: xgene: defconfig: Enable Standby GPIO
From: Iyappan Subramanian @ 2016-10-06 21:35 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1475789758-5196-1-git-send-email-isubramanian@apm.com>
Enable CONFIG_GPIO_XGENE_SB.
Signed-off-by: Iyappan Subramanian <isubramanian@apm.com>
Signed-off-by: Quan Nguyen <qnguyen@apm.com>
---
arch/arm64/configs/defconfig | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
index eadf485..be52a00 100644
--- a/arch/arm64/configs/defconfig
+++ b/arch/arm64/configs/defconfig
@@ -240,6 +240,7 @@ CONFIG_GPIO_DWAPB=y
CONFIG_GPIO_PL061=y
CONFIG_GPIO_RCAR=y
CONFIG_GPIO_XGENE=y
+CONFIG_GPIO_XGENE_SB=y
CONFIG_GPIO_PCA953X=y
CONFIG_GPIO_PCA953X_IRQ=y
CONFIG_GPIO_MAX77620=y
--
1.9.1
^ permalink raw reply related
* [PATCH net-next 1/2] drivers: net: xgene: fix: Use GPIO to get link status
From: Iyappan Subramanian @ 2016-10-06 21:35 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1475789758-5196-1-git-send-email-isubramanian@apm.com>
The link value reported by the link status register is not
reliable when no SPF module inserted. This patchset fixes this
issue by using GPIO to determine the link status.
Signed-off-by: Iyappan Subramanian <isubramanian@apm.com>
Signed-off-by: Quan Nguyen <qnguyen@apm.com>
---
drivers/net/ethernet/apm/xgene/xgene_enet_main.c | 6 +++++-
drivers/net/ethernet/apm/xgene/xgene_enet_main.h | 1 +
drivers/net/ethernet/apm/xgene/xgene_enet_xgmac.c | 19 +++++++++++++++++--
3 files changed, 23 insertions(+), 3 deletions(-)
diff --git a/drivers/net/ethernet/apm/xgene/xgene_enet_main.c b/drivers/net/ethernet/apm/xgene/xgene_enet_main.c
index 429f18f..f75d955 100644
--- a/drivers/net/ethernet/apm/xgene/xgene_enet_main.c
+++ b/drivers/net/ethernet/apm/xgene/xgene_enet_main.c
@@ -1381,9 +1381,13 @@ static void xgene_enet_gpiod_get(struct xgene_enet_pdata *pdata)
{
struct device *dev = &pdata->pdev->dev;
- if (pdata->phy_mode != PHY_INTERFACE_MODE_XGMII)
+ pdata->sfp_gpio_en = false;
+ if (pdata->phy_mode != PHY_INTERFACE_MODE_XGMII ||
+ (!device_property_present(dev, "sfp-gpios") &&
+ !device_property_present(dev, "rxlos-gpios")))
return;
+ pdata->sfp_gpio_en = true;
pdata->sfp_rdy = gpiod_get(dev, "rxlos", GPIOD_IN);
if (IS_ERR(pdata->sfp_rdy))
pdata->sfp_rdy = gpiod_get(dev, "sfp", GPIOD_IN);
diff --git a/drivers/net/ethernet/apm/xgene/xgene_enet_main.h b/drivers/net/ethernet/apm/xgene/xgene_enet_main.h
index 0cda58f..011965b 100644
--- a/drivers/net/ethernet/apm/xgene/xgene_enet_main.h
+++ b/drivers/net/ethernet/apm/xgene/xgene_enet_main.h
@@ -219,6 +219,7 @@ struct xgene_enet_pdata {
u8 rx_delay;
bool mdio_driver;
struct gpio_desc *sfp_rdy;
+ bool sfp_gpio_en;
};
struct xgene_indirect_ctl {
diff --git a/drivers/net/ethernet/apm/xgene/xgene_enet_xgmac.c b/drivers/net/ethernet/apm/xgene/xgene_enet_xgmac.c
index 6475f38..d1758b0 100644
--- a/drivers/net/ethernet/apm/xgene/xgene_enet_xgmac.c
+++ b/drivers/net/ethernet/apm/xgene/xgene_enet_xgmac.c
@@ -415,16 +415,31 @@ static void xgene_enet_clear(struct xgene_enet_pdata *pdata,
xgene_enet_wr_ring_if(pdata, addr, data);
}
+static int xgene_enet_gpio_lookup(struct xgene_enet_pdata *pdata)
+{
+ struct device *dev = &pdata->pdev->dev;
+
+ pdata->sfp_rdy = gpiod_get(dev, "rxlos", GPIOD_IN);
+ if (IS_ERR(pdata->sfp_rdy))
+ pdata->sfp_rdy = gpiod_get(dev, "sfp", GPIOD_IN);
+
+ if (IS_ERR(pdata->sfp_rdy))
+ return -ENODEV;
+
+ return 0;
+}
+
static void xgene_enet_link_state(struct work_struct *work)
{
struct xgene_enet_pdata *pdata = container_of(to_delayed_work(work),
struct xgene_enet_pdata, link_work);
- struct gpio_desc *sfp_rdy = pdata->sfp_rdy;
struct net_device *ndev = pdata->ndev;
u32 link_status, poll_interval;
link_status = xgene_enet_link_status(pdata);
- if (link_status && !IS_ERR(sfp_rdy) && !gpiod_get_value(sfp_rdy))
+ if (pdata->sfp_gpio_en && link_status &&
+ (!IS_ERR(pdata->sfp_rdy) || !xgene_enet_gpio_lookup(pdata)) &&
+ !gpiod_get_value(pdata->sfp_rdy))
link_status = 0;
if (link_status) {
--
1.9.1
^ permalink raw reply related
* [PATCH net-next 0/2] drivers: net: xgene: fix: Use GPIO to get link status
From: Iyappan Subramanian @ 2016-10-06 21:35 UTC (permalink / raw)
To: linux-arm-kernel
Since the link value reported by the link status register is not
reliable if no SPF module inserted, this patchset fixes the issue by
using GPIO to determine the link status when no module inserted.
Signed-off-by: Iyappan Subramanian <isubramanian@apm.com>
Signed-off-by: Quan Nguyen <qnguyen@apm.com>
---
Iyappan Subramanian (2):
drivers: net: xgene: fix: Use GPIO to get link status
arm64: xgene: defconfig: Enable Standby GPIO
arch/arm64/configs/defconfig | 1 +
drivers/net/ethernet/apm/xgene/xgene_enet_main.c | 6 +++++-
drivers/net/ethernet/apm/xgene/xgene_enet_main.h | 1 +
drivers/net/ethernet/apm/xgene/xgene_enet_xgmac.c | 19 +++++++++++++++++--
4 files changed, 24 insertions(+), 3 deletions(-)
--
1.9.1
^ permalink raw reply
* [PATCH] Revert "debugfs: ->d_parent is never NULL or negative"
From: Sinan Kaya @ 2016-10-06 21:30 UTC (permalink / raw)
To: linux-arm-kernel
This reverts commit acc29fb8f792 ("debugfs: ->d_parent is never NULL or
negative") as it breaks the debugfs_remove_recursive API as show in the
callstack below.
Tested against:
c802e87 Add linux-next specific files for 20161006
Unable to handle kernel NULL pointer dereference at virtual address
000000a8
pgd = ffff800bc81c1000
[000000a8] *pgd=0000004bc83e1003, *pud=0000004bc6519003,
*pmd=0000000000000000
Internal error: Oops: 96000006 [#1] PREEMPT SMP
Modules linked in:
CPU: 13 PID: 1758 Comm: tee Not tainted 4.8.0-next-20161006-00013-gd66147c
Hardware name: (null) (DT)
task: ffff800bc7fe0000 task.stack: ffff800bc645c000
PC is at down_write+0x18/0x68
LR is at debugfs_remove_recursive+0x50/0x1c0
pc : [<ffff00000889f480>] lr : [<ffff00000831c220>] pstate: 80000145
[<ffff00000889f480>] down_write+0x18/0x68
[<ffff00000831c220>] debugfs_remove_recursive+0x50/0x1c0
[<ffff00000848e0a8>] hidma_debug_uninit+0x20/0x30
[<ffff00000848c5d8>] hidma_remove+0x48/0x98
[<ffff000008511b6c>] platform_drv_remove+0x24/0x68
[<ffff00000850fac8>] __device_release_driver+0x80/0x118
[<ffff00000850fb84>] device_release_driver+0x24/0x38
[<ffff00000850e928>] unbind_store+0xe8/0x110
[<ffff00000850dd30>] drv_attr_store+0x20/0x30
[<ffff000008253a48>] sysfs_kf_write+0x48/0x58
[<ffff000008252dd8>] kernfs_fop_write+0xb0/0x1d8
[<ffff0000081dab3c>] __vfs_write+0x1c/0x110
[<ffff0000081db940>] vfs_write+0xa0/0x1b8
[<ffff0000081dcd34>] SyS_write+0x44/0xa0
[<ffff000008082ef0>] el0_svc_naked+0x24/0x28
Reverting this change seems to fix the issue.
Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
---
fs/debugfs/inode.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c
index f17fcf8..02166d6 100644
--- a/fs/debugfs/inode.c
+++ b/fs/debugfs/inode.c
@@ -621,6 +621,9 @@ void debugfs_remove(struct dentry *dentry)
return;
parent = dentry->d_parent;
+ if (!parent || d_really_is_negative(parent))
+ return;
+
inode_lock(d_inode(parent));
ret = __debugfs_remove(dentry, parent);
inode_unlock(d_inode(parent));
@@ -651,6 +654,10 @@ void debugfs_remove_recursive(struct dentry *dentry)
if (IS_ERR_OR_NULL(dentry))
return;
+ parent = dentry->d_parent;
+ if (!parent || d_really_is_negative(parent))
+ return;
+
parent = dentry;
down:
inode_lock(d_inode(parent));
--
1.9.1
^ permalink raw reply related
* [PATCH v5 2/5] drm/bridge: Add RGB to VGA bridge support
From: Laurent Pinchart @ 2016-10-06 21:04 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CAOw6vbLC=HnnpuaxFf8EzSTmuYoZd-r1aqx-ixja97Ht_U6S6Q@mail.gmail.com>
Hi Sean,
On Thursday 06 Oct 2016 15:53:28 Sean Paul wrote:
> On Thu, Oct 6, 2016 at 1:27 PM, Laurent Pinchart wrote:
> > On Thursday 06 Oct 2016 17:09:57 Archit Taneja wrote:
> >> On 10/06/2016 12:51 PM, Maxime Ripard wrote:
> >>> On Mon, Oct 03, 2016 at 04:40:57PM +0530, Archit Taneja wrote:
> >>>> On 09/30/2016 08:07 PM, Maxime Ripard wrote:
> >>>>> Some boards have an entirely passive RGB to VGA bridge, based on
> >>>>> either DACs or resistor ladders.
> >>>>>
> >>>>> Those might or might not have an i2c bus routed to the VGA connector
> >>>>> in order to access the screen EDIDs.
> >>>>>
> >>>>> Add a bridge that doesn't do anything but expose the modes available
> >>>>> on the screen, either based on the EDIDs if available, or based on
> >>>>> the XGA standards.
> >>>>>
> >>>>> Acked-by: Rob Herring <robh@kernel.org>
> >>>>> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> >>>>> ---
> >>>>> .../bindings/display/bridge/rgb-to-vga-bridge.txt | 48 +++++
> >>>>> drivers/gpu/drm/bridge/Kconfig | 7 +
> >>>>> drivers/gpu/drm/bridge/Makefile | 1 +
> >>>>> drivers/gpu/drm/bridge/rgb-to-vga.c | 229 +++++++++++++
> >>>>> 4 files changed, 285 insertions(+)
> >>>>> create mode 100644
> >>>>> Documentation/devicetree/bindings/display/bridge/rgb-to-vga-bridge.tx
> >>>>> t
> >>>>> create mode 100644 drivers/gpu/drm/bridge/rgb-to-vga.c
> >>>>>
> >>>>> diff --git
> >>>>> a/Documentation/devicetree/bindings/display/bridge/rgb-to-vga-bridge.
> >>>>> txt
> >>>>> b/Documentation/devicetree/bindings/display/bridge/rgb-to-vga-bridge.
> >>>>> txt
> >>>>> new file mode 100644
> >>>>> index 000000000000..a8375bc1f9cb
> >>>>> --- /dev/null
> >>>>> +++
> >>>>> b/Documentation/devicetree/bindings/display/bridge/rgb-to-vga-bridge.
> >>>>> tx
> >>>>> t @@ -0,0 +1,48 @@
> >>>>> +Dumb RGB to VGA bridge
> >>>>> +----------------------
> >>>>> +
> >>>>> +This binding is aimed for dumb RGB to VGA bridges that do not
> >>>>> require
> >>>>> +any configuration.
> >>>>> +
> >>>>> +Required properties:
> >>>>> +
> >>>>> +- compatible: Must be "rgb-to-vga-bridge"
> >>>>
> >>>> I'd talked to Laurent on IRC if he's okay with this. And I guess you
> >>>> to had discussed it during XDC too. He's suggested that it'd be better
> >>>> to have the compatible string as "simple-vga-dac".
> >>>
> >>> I just wished this bikeshedding had taken place publicly and be
> >>> actually part of that discussion, but yeah, ok.
> >>
> >> Sorry about that. I'd pinged him for an Ack, the discussion went
> >> more than that :)
> >>
> >>>> Some of the reasons behind having this:
> >>>>
> >>>> - We don't need to specify "rgb" in the compatible string since most
> >>>> simple VGA DACs can only work with an RGB input.
> >>>
> >>> Ok.
> >>>
> >>>> - Also, with "dac" specified in the string, we don't need to
> >>>> specifically mention "bridge" in the string. Also, bridge is a drm
> >>>> specific term.
> >>>>
> >>>> - "simple" is considered because it's an unconfigurable bridge, and it
> >>>> might be misleading for other VGA DACs to not use "vga-dac".
> >>>
> >>> All those "simple" bindings are just the biggest lie we ever
> >>> told. It's simple when you introduce it, and then grows into something
> >>> much more complicated than a non-simple implementation.
> >>
> >> "simple" here is supposed to mean that it's an unconfigurable RGB to
> >> VGA DAC. This isn't supposed to follow the simple-panel model, where
> >> you add the "simple-panel" string in the compatible node, along with
> >> you chip specific compatible string.
> >
> > I agree with Maxime, I don't like the word "simple". My preference would
> > be "vga-dac" for a lack of a better qualifier than "simple" to describe
> > the fact that the device requires no configuration. My only concern with
> > "vga-dac" is that we would restrict usage of that compatible string for a
> > subset of VGA DACs, with more complex devices not being compatible with
> > "vga-dac" even though they are VGA DACs. That's a problem I can live with
> > though.
>
> While we're bikeshedding (feel free to ignore my input on this), I
> think Maxime's initial "dumb" qualifier was better than "simple".
I think I agree.
> I think "passive" also gets the point across better than "simple", which
> we've already established as something else in drm.
To my electrical engineer's ear, passive refers to a component or combination
of components that is not capable of power gain. The resistors ladder VGA DAC
that Maxime is trying to support is a passive system, but the ADV7123 VGA DAC
that equally requires no configuration is an active component.
> Now that I've gotten that out of the way, this patch looks good to me
> regardless of the name.
>
> Reviewed-by: Sean Paul <seanpaul@chromium.org>
>
> Sean
>
> >> In other words, this driver shouldn't be touched again in the future :)
> >> If someone wants to write a RGB to VGA driver which is even
> >> slightly configurable, they'll need to write a new bridge driver.
> >
> > I'm sure that won't be true. I can certainly foresee the addition of
> > regulators support for instance. It's unfortunately never black and white.
> >
> >>>> What do you think about this? If you think it's good, would it be
> >>>> possible for you to change this? I guess it's okay for the rest of
> >>>> the patch to stay the same.
> >>>
> >>> I'll update and respin the serie.
--
Regards,
Laurent Pinchart
^ permalink raw reply
* [PATCH 1/8] PM / Domains: Make genpd state allocation dynamic
From: Lina Iyer @ 2016-10-06 20:57 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CAPDyKFps5kZLcVW1UJsZtgOQEysRxc9tj41uDDTeJav8YBQG4A@mail.gmail.com>
On Thu, Oct 06 2016 at 13:45 -0600, Ulf Hansson wrote:
>On 6 October 2016 at 17:40, Lina Iyer <lina.iyer@linaro.org> wrote:
>> On Thu, Oct 06 2016 at 02:37 -0600, Ulf Hansson wrote:
>>>
>>> On 5 October 2016 at 22:31, Lina Iyer <lina.iyer@linaro.org> wrote:
>>>>
>>>> Allow PM Domain states to be defined dynamically by the drivers. This
>>>> removes the limitation on the maximum number of states possible for a
>>>> domain.
>>>>
>>>> Cc: Axel Haslam <ahaslam+renesas@baylibre.com>
>>>> Suggested-by: Ulf Hansson <ulf.hansson@linaro.org>
>>>> Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
>>
>> <...>
>>>>
>>>> -#define GENPD_MAX_NUM_STATES 8 /* Number of possible low power states
>>>> */
>>>> -
>>>> enum gpd_status {
>>>> GPD_STATE_ACTIVE = 0, /* PM domain is active */
>>>> GPD_STATE_POWER_OFF, /* PM domain is off */
>>>> @@ -70,7 +68,7 @@ struct generic_pm_domain {
>>>> void (*detach_dev)(struct generic_pm_domain *domain,
>>>> struct device *dev);
>>>> unsigned int flags; /* Bit field of configs for genpd
>>>> */
>>>> - struct genpd_power_state states[GENPD_MAX_NUM_STATES];
>>>> + struct genpd_power_state *states;
>>>> unsigned int state_count; /* number of states */
>>>> unsigned int state_idx; /* state that genpd will go to when off
>>>> */
>>>>
>>>> --
>>>> 2.7.4
>>>>
>>>
>>> In general I like the improvement, but..
>>>
>>> This change implies that ->states may very well be NULL. This isn't
>>> validated by genpd's internal logic when power off/on the domain
>>> (genpd_power_on|off(), __default_power_down_ok()). You need to fix
>>> this, somehow.
>>>
>> Good point.
>>
>>> Perhaps the easiest solutions is, when pm_genpd_init() finds that
>>> ->state is NULL, that we allocate a struct genpd_power_state with
>>> array size of 1 and assign it to ->states. Although, doing this also
>>> means you need to track that genpd was responsible for the the
>>> allocation, so it must also free the data from within genpd_remove().
>>>
>>> Unless you have other ideas!?
>>>
>> I can think of some hacks, but they are uglier than the problem we are
>> trying to solve. We could drop this patch. Real world situations would
>> not have more than 8 states and if there is one, we can think about it
>> then.
>
>The problem with the current approach is that we waste some memory as
>we always have an array of 8 states per genpd. In the worst case,
>which currently is the most common case, only 1 out of 8 states is
>being used.
>
>So, let's not be lazy here and instead take the opportunity to fix
>this, and especially I think this makes sense, before we go on and add
>the DT parsing of the domain-idle-states.
>
Hmm.. We are not wasting much memory in comparison, but if you insist,
sure.
>The more sophisticated method would probably be to use kobject/kref,
>but let's not go there for now. Instead let's try an easy method of
>just tracking whether the allocations had been made internally by
>genpd, via adding a "bool state_allocated to the struct
>generic_pm_domain. Would that work?
>
It would work.
i.
How about an additional static state by default in the domain structure,
if the platform does not provide a state then the default structure is
used. That way we dont have to track it. But it does waste memory
eqivalent to a state, when there are states provided by the platform.
ii.
I could add a void *free to the domain structure and save the memory
allocated by default in the *free. At domain remove, we just do a kfree
on free.
iii.
Use a boolean flag.
Thanks,
Lina
^ permalink raw reply
* [PATCH v13 15/15] vfio/type1: Return the MSI geometry through VFIO_IOMMU_GET_INFO capability chains
From: Alex Williamson @ 2016-10-06 20:42 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161006142040.13cbdb8f@t450s.home>
On Thu, 6 Oct 2016 14:20:40 -0600
Alex Williamson <alex.williamson@redhat.com> wrote:
> On Thu, 6 Oct 2016 08:45:31 +0000
> Eric Auger <eric.auger@redhat.com> wrote:
>
> > This patch allows the user-space to retrieve the MSI geometry. The
> > implementation is based on capability chains, now also added to
> > VFIO_IOMMU_GET_INFO.
> >
> > The returned info comprise:
> > - whether the MSI IOVA are constrained to a reserved range (x86 case) and
> > in the positive, the start/end of the aperture,
> > - or whether the IOVA aperture need to be set by the userspace. In that
> > case, the size and alignment of the IOVA window to be provided are
> > returned.
> >
> > In case the userspace must provide the IOVA aperture, we currently report
> > a size/alignment based on all the doorbells registered by the host kernel.
> > This may exceed the actual needs.
> >
> > Signed-off-by: Eric Auger <eric.auger@redhat.com>
> >
> > ---
> > v11 -> v11:
> > - msi_doorbell_pages was renamed msi_doorbell_calc_pages
> >
> > v9 -> v10:
> > - move cap_offset after iova_pgsizes
> > - replace __u64 alignment by __u32 order
> > - introduce __u32 flags in vfio_iommu_type1_info_cap_msi_geometry and
> > fix alignment
> > - call msi-doorbell API to compute the size/alignment
> >
> > v8 -> v9:
> > - use iommu_msi_supported flag instead of programmable
> > - replace IOMMU_INFO_REQUIRE_MSI_MAP flag by a more sophisticated
> > capability chain, reporting the MSI geometry
> >
> > v7 -> v8:
> > - use iommu_domain_msi_geometry
> >
> > v6 -> v7:
> > - remove the computation of the number of IOVA pages to be provisionned.
> > This number depends on the domain/group/device topology which can
> > dynamically change. Let's rely instead rely on an arbitrary max depending
> > on the system
> >
> > v4 -> v5:
> > - move msi_info and ret declaration within the conditional code
> >
> > v3 -> v4:
> > - replace former vfio_domains_require_msi_mapping by
> > more complex computation of MSI mapping requirements, especially the
> > number of pages to be provided by the user-space.
> > - reword patch title
> >
> > RFC v1 -> v1:
> > - derived from
> > [RFC PATCH 3/6] vfio: Extend iommu-info to return MSIs automap state
> > - renamed allow_msi_reconfig into require_msi_mapping
> > - fixed VFIO_IOMMU_GET_INFO
> > ---
> > drivers/vfio/vfio_iommu_type1.c | 78 ++++++++++++++++++++++++++++++++++++++++-
> > include/uapi/linux/vfio.h | 32 ++++++++++++++++-
> > 2 files changed, 108 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
> > index dc3ee5d..ce5e7eb 100644
> > --- a/drivers/vfio/vfio_iommu_type1.c
> > +++ b/drivers/vfio/vfio_iommu_type1.c
> > @@ -38,6 +38,8 @@
> > #include <linux/workqueue.h>
> > #include <linux/dma-iommu.h>
> > #include <linux/msi-doorbell.h>
> > +#include <linux/irqdomain.h>
> > +#include <linux/msi.h>
> >
> > #define DRIVER_VERSION "0.2"
> > #define DRIVER_AUTHOR "Alex Williamson <alex.williamson@redhat.com>"
> > @@ -1101,6 +1103,55 @@ static int vfio_domains_have_iommu_cache(struct vfio_iommu *iommu)
> > return ret;
> > }
> >
> > +static int compute_msi_geometry_caps(struct vfio_iommu *iommu,
> > + struct vfio_info_cap *caps)
> > +{
> > + struct vfio_iommu_type1_info_cap_msi_geometry *vfio_msi_geometry;
> > + unsigned long order = __ffs(vfio_pgsize_bitmap(iommu));
> > + struct iommu_domain_msi_geometry msi_geometry;
> > + struct vfio_info_cap_header *header;
> > + struct vfio_domain *d;
> > + bool reserved;
> > + size_t size;
> > +
> > + mutex_lock(&iommu->lock);
> > + /* All domains have same require_msi_map property, pick first */
> > + d = list_first_entry(&iommu->domain_list, struct vfio_domain, next);
> > + iommu_domain_get_attr(d->domain, DOMAIN_ATTR_MSI_GEOMETRY,
> > + &msi_geometry);
> > + reserved = !msi_geometry.iommu_msi_supported;
> > +
> > + mutex_unlock(&iommu->lock);
> > +
> > + size = sizeof(*vfio_msi_geometry);
> > + header = vfio_info_cap_add(caps, size,
> > + VFIO_IOMMU_TYPE1_INFO_CAP_MSI_GEOMETRY, 1);
> > +
> > + if (IS_ERR(header))
> > + return PTR_ERR(header);
> > +
> > + vfio_msi_geometry = container_of(header,
> > + struct vfio_iommu_type1_info_cap_msi_geometry,
> > + header);
> > +
> > + vfio_msi_geometry->flags = reserved;
>
> Use the bit flag VFIO_IOMMU_MSI_GEOMETRY_RESERVED
>
> > + if (reserved) {
> > + vfio_msi_geometry->aperture_start = msi_geometry.aperture_start;
> > + vfio_msi_geometry->aperture_end = msi_geometry.aperture_end;
>
> But maybe nobody has set these, did you intend to use
> iommu_domain_msi_aperture_valid(), which you defined early on but never
> used?
>
> > + return 0;
> > + }
> > +
> > + vfio_msi_geometry->order = order;
>
> I'm tempted to suggest that a user could do the same math on their own
> since we provide the supported bitmap already... could it ever not be
> the same?
>
> > + /*
> > + * we compute a system-wide requirement based on all the registered
> > + * doorbells
> > + */
> > + vfio_msi_geometry->size =
> > + msi_doorbell_calc_pages(order) * ((uint64_t) 1 << order);
> > +
> > + return 0;
> > +}
> > +
> > static long vfio_iommu_type1_ioctl(void *iommu_data,
> > unsigned int cmd, unsigned long arg)
> > {
> > @@ -1122,8 +1173,10 @@ static long vfio_iommu_type1_ioctl(void *iommu_data,
> > }
> > } else if (cmd == VFIO_IOMMU_GET_INFO) {
> > struct vfio_iommu_type1_info info;
> > + struct vfio_info_cap caps = { .buf = NULL, .size = 0 };
> > + int ret;
> >
> > - minsz = offsetofend(struct vfio_iommu_type1_info, iova_pgsizes);
> > + minsz = offsetofend(struct vfio_iommu_type1_info, cap_offset);
> >
> > if (copy_from_user(&info, (void __user *)arg, minsz))
> > return -EFAULT;
> > @@ -1135,6 +1188,29 @@ static long vfio_iommu_type1_ioctl(void *iommu_data,
> >
> > info.iova_pgsizes = vfio_pgsize_bitmap(iommu);
> >
> > + ret = compute_msi_geometry_caps(iommu, &caps);
> > + if (ret)
> > + return ret;
> > +
> > + if (caps.size) {
> > + info.flags |= VFIO_IOMMU_INFO_CAPS;
> > + if (info.argsz < sizeof(info) + caps.size) {
> > + info.argsz = sizeof(info) + caps.size;
> > + info.cap_offset = 0;
> > + } else {
> > + vfio_info_cap_shift(&caps, sizeof(info));
> > + if (copy_to_user((void __user *)arg +
> > + sizeof(info), caps.buf,
> > + caps.size)) {
> > + kfree(caps.buf);
> > + return -EFAULT;
> > + }
> > + info.cap_offset = sizeof(info);
> > + }
> > +
> > + kfree(caps.buf);
> > + }
> > +
> > return copy_to_user((void __user *)arg, &info, minsz) ?
> > -EFAULT : 0;
> >
> > diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
> > index 4a9dbc2..8dae013 100644
> > --- a/include/uapi/linux/vfio.h
> > +++ b/include/uapi/linux/vfio.h
> > @@ -488,7 +488,35 @@ struct vfio_iommu_type1_info {
> > __u32 argsz;
> > __u32 flags;
> > #define VFIO_IOMMU_INFO_PGSIZES (1 << 0) /* supported page sizes info */
> > - __u64 iova_pgsizes; /* Bitmap of supported page sizes */
> > +#define VFIO_IOMMU_INFO_CAPS (1 << 1) /* Info supports caps */
> > + __u64 iova_pgsizes; /* Bitmap of supported page sizes */
> > + __u32 __resv;
> > + __u32 cap_offset; /* Offset within info struct of first cap */
> > +};
>
> I understand the padding, but not the ordering. Why not end with
> padding?
>
> > +
> > +#define VFIO_IOMMU_TYPE1_INFO_CAP_MSI_GEOMETRY 1
> > +
> > +/*
> > + * The MSI geometry capability allows to report the MSI IOVA geometry:
> > + * - either the MSI IOVAs are constrained within a reserved IOVA aperture
> > + * whose boundaries are given by [@aperture_start, @aperture_end].
> > + * this is typically the case on x86 host. The userspace is not allowed
> > + * to map userspace memory at IOVAs intersecting this range using
> > + * VFIO_IOMMU_MAP_DMA.
> > + * - or the MSI IOVAs are not requested to belong to any reserved range;
> > + * in that case the userspace must provide an IOVA window characterized by
> > + * @size and @alignment using VFIO_IOMMU_MAP_DMA with RESERVED_MSI_IOVA flag.
> > + */
> > +struct vfio_iommu_type1_info_cap_msi_geometry {
> > + struct vfio_info_cap_header header;
> > + __u32 flags;
> > +#define VFIO_IOMMU_MSI_GEOMETRY_RESERVED (1 << 0) /* reserved geometry */
> > + /* not reserved */
> > + __u32 order; /* iommu page order used for aperture alignment*/
> > + __u64 size; /* IOVA aperture size (bytes) the userspace must provide */
> > + /* reserved */
> > + __u64 aperture_start;
> > + __u64 aperture_end;
>
> Should these be a union? We never set them both. Should the !reserved
> case have a flag as well, so the user can positively identify what's
> being provided?
Actually, is there really any need to fit both of these within the same
structure? Part of the idea of the capability chains is we can create
a capability for each new thing we want to describe. So, we could
simply define a generic reserved IOVA range capability with a 'start'
and 'end' and then another capability to define MSI mapping
requirements. Thanks,
Alex
> > };
> >
> > #define VFIO_IOMMU_GET_INFO _IO(VFIO_TYPE, VFIO_BASE + 12)
> > @@ -503,6 +531,8 @@ struct vfio_iommu_type1_info {
> > * IOVA region that will be used on some platforms to map the host MSI frames.
> > * In that specific case, vaddr is ignored. Once registered, an MSI reserved
> > * IOVA region stays until the container is closed.
> > + * The requirement for provisioning such reserved IOVA range can be checked by
> > + * checking the VFIO_IOMMU_TYPE1_INFO_CAP_MSI_GEOMETRY capability.
> > */
> > struct vfio_iommu_type1_dma_map {
> > __u32 argsz;
>
^ permalink raw reply
* [PATCH 3/7] mfd: db8500-prcmu: Remove unused *prcmu_set_ddr_opp() calls
From: Ulf Hansson @ 2016-10-06 20:35 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20160915104521.14286-4-lee.jones@linaro.org>
On 15 September 2016 at 12:45, Lee Jones <lee.jones@linaro.org> wrote:
> There are no call sites for these functions. Strip them out.
>
> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> ---
> drivers/mfd/db8500-prcmu.c | 19 -------------------
> include/linux/mfd/db8500-prcmu.h | 6 ------
> include/linux/mfd/dbx500-prcmu.h | 9 ---------
> 3 files changed, 34 deletions(-)
>
> diff --git a/drivers/mfd/db8500-prcmu.c b/drivers/mfd/db8500-prcmu.c
> index 388e268..ca38a6a 100644
> --- a/drivers/mfd/db8500-prcmu.c
> +++ b/drivers/mfd/db8500-prcmu.c
> @@ -938,25 +938,6 @@ int db8500_prcmu_get_ddr_opp(void)
> return readb(PRCM_DDR_SUBSYS_APE_MINBW);
> }
>
> -/**
> - * db8500_set_ddr_opp - set the appropriate DDR OPP
> - * @opp: The new DDR operating point to which transition is to be made
> - * Returns: 0 on success, non-zero on failure
> - *
> - * This function sets the operating point of the DDR.
> - */
> -static bool enable_set_ddr_opp;
> -int db8500_prcmu_set_ddr_opp(u8 opp)
> -{
> - if (opp < DDR_100_OPP || opp > DDR_25_OPP)
> - return -EINVAL;
> - /* Changing the DDR OPP can hang the hardware pre-v21 */
> - if (enable_set_ddr_opp)
> - writeb(opp, PRCM_DDR_SUBSYS_APE_MINBW);
> -
> - return 0;
> -}
> -
This function was actually used in the vendor tree to deal with DVFS,
implemented a system wide PM QoS thing.
I guess those hacks never made it upstream. :-)
> /* Divide the frequency of certain clocks by 2 for APE_50_PARTLY_25_OPP. */
> static void request_even_slower_clocks(bool enable)
> {
> diff --git a/include/linux/mfd/db8500-prcmu.h b/include/linux/mfd/db8500-prcmu.h
> index 0bd6944..7ba67b5 100644
> --- a/include/linux/mfd/db8500-prcmu.h
> +++ b/include/linux/mfd/db8500-prcmu.h
> @@ -538,7 +538,6 @@ int db8500_prcmu_get_arm_opp(void);
> int db8500_prcmu_set_ape_opp(u8 opp);
> int db8500_prcmu_get_ape_opp(void);
> int db8500_prcmu_request_ape_opp_100_voltage(bool enable);
> -int db8500_prcmu_set_ddr_opp(u8 opp);
> int db8500_prcmu_get_ddr_opp(void);
>
> u32 db8500_prcmu_read(unsigned int reg);
> @@ -594,11 +593,6 @@ static inline int prcmu_release_usb_wakeup_state(void)
> return 0;
> }
>
> -static inline int db8500_prcmu_set_ddr_opp(u8 opp)
> -{
> - return 0;
> -}
> -
> static inline int db8500_prcmu_get_ddr_opp(void)
> {
> return DDR_100_OPP;
> diff --git a/include/linux/mfd/dbx500-prcmu.h b/include/linux/mfd/dbx500-prcmu.h
> index 5d37460..2e2c6a6 100644
> --- a/include/linux/mfd/dbx500-prcmu.h
> +++ b/include/linux/mfd/dbx500-prcmu.h
> @@ -269,10 +269,6 @@ unsigned long prcmu_clock_rate(u8 clock);
> long prcmu_round_clock_rate(u8 clock, unsigned long rate);
> int prcmu_set_clock_rate(u8 clock, unsigned long rate);
>
> -static inline int prcmu_set_ddr_opp(u8 opp)
> -{
> - return db8500_prcmu_set_ddr_opp(opp);
> -}
> static inline int prcmu_get_ddr_opp(void)
> {
> return db8500_prcmu_get_ddr_opp();
> @@ -489,11 +485,6 @@ static inline int prcmu_get_arm_opp(void)
> return ARM_100_OPP;
> }
>
> -static inline int prcmu_set_ddr_opp(u8 opp)
> -{
> - return 0;
> -}
> -
> static inline int prcmu_get_ddr_opp(void)
> {
> return DDR_100_OPP;
> --
> 2.9.3
>
If not too late:
Acked-by: Ulf Hansson <ulf.hansson@linaro.org>
Kind regards
Uffe
^ permalink raw reply
* [PATCH 3/3] ARM: dts: imx6q-evi: support cyclonespi
From: Joshua Clayton @ 2016-10-06 20:34 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <4b4432c04b4ea92a2af814e3d7866c33f2eb12ea.1475783742.git.stillcompiling@gmail.com>
Add support for Altera cyclone V FPGA connected to an spi port
Signed-off-by: Joshua Clayton <stillcompiling@gmail.com>
---
arch/arm/boot/dts/imx6q-evi.dts | 16 ++++++++++++++++
1 file changed, 16 insertions(+)
diff --git a/arch/arm/boot/dts/imx6q-evi.dts b/arch/arm/boot/dts/imx6q-evi.dts
index 6de21ff..7e5d3cf 100644
--- a/arch/arm/boot/dts/imx6q-evi.dts
+++ b/arch/arm/boot/dts/imx6q-evi.dts
@@ -95,6 +95,15 @@
pinctrl-names = "default";
pinctrl-0 = <&pinctrl_ecspi1 &pinctrl_ecspi1cs>;
status = "okay";
+
+ fpga_spi: cyclonespi at 0 {
+ compatible = "altr,cyclonespi-fpga-mgr";
+ spi-max-frequency = <20000000>;
+ reg = <0>;
+ pinctrl-0 = <&pinctrl_fpgaspi>;
+ reset-gpio = <&gpio4 9 GPIO_ACTIVE_HIGH>;
+ status-gpio = <&gpio4 11 GPIO_ACTIVE_HIGH>;
+ };
};
&ecspi3 {
@@ -325,6 +334,13 @@
>;
};
+ pinctrl_fpgaspi: fpgaspigrp {
+ fsl,pins = <
+ MX6QDL_PAD_KEY_ROW1__GPIO4_IO09 0x1b0b0
+ MX6QDL_PAD_KEY_ROW2__GPIO4_IO11 0x1b0b0
+ >;
+ };
+
pinctrl_gpminand: gpminandgrp {
fsl,pins = <
MX6QDL_PAD_NANDF_CLE__NAND_CLE 0xb0b1
--
2.7.4
^ permalink raw reply related
* [PATCH 2/3] doc: dt: add cyclone-spi binding document
From: Joshua Clayton @ 2016-10-06 20:34 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <4b4432c04b4ea92a2af814e3d7866c33f2eb12ea.1475783742.git.stillcompiling@gmail.com>
Describe a cyclonespi devicetree entry, required features
Signed-off-by: Joshua Clayton <stillcompiling@gmail.com>
---
.../bindings/fpga/cyclone-spi-fpga-mgr.txt | 23 ++++++++++++++++++++++
1 file changed, 23 insertions(+)
create mode 100644 Documentation/devicetree/bindings/fpga/cyclone-spi-fpga-mgr.txt
diff --git a/Documentation/devicetree/bindings/fpga/cyclone-spi-fpga-mgr.txt b/Documentation/devicetree/bindings/fpga/cyclone-spi-fpga-mgr.txt
new file mode 100644
index 0000000..8de34db
--- /dev/null
+++ b/Documentation/devicetree/bindings/fpga/cyclone-spi-fpga-mgr.txt
@@ -0,0 +1,23 @@
+Altera SOCFPGA FPGA Manager
+
+Altera cyclone FPGAs support a method of loading the bitstream over what is
+referred to as "passive serial".
+The passive serial link is not technically spi, and might require extra
+circuits in order to play nicely with other spi slaves on the same bus.
+
+See https://www.altera.com/literature/hb/cyc/cyc_c51013.pdf
+
+Required properties:
+- compatible : should contain "altr,cyclonespi-fpga-mgr"
+- reg : spi slave id of the fpga
+- reset-gpio : reset pin (referred to as nCONFIG in the cyclone manual)
+- status-gpio : status pin (referred to as nSTATUS in the cyclone manual)
+
+Example:
+ fpga_spi: evi-fpga-spi at 0 {
+ compatible = "altr,cyclonespi-fpga-mgr";
+ spi-max-frequency = <20000000>;
+ reg = <0>;
+ reset-gpio = <&gpio4 9 GPIO_ACTIVE_HIGH>;
+ status-gpio = <&gpio4 11 GPIO_ACTIVE_HIGH>;
+ };
--
2.7.4
^ permalink raw reply related
* [PATCH 1/3] fpga manager: Add cyclonespi driver for Altera fpgas
From: Joshua Clayton @ 2016-10-06 20:34 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <cover.1475783742.git.stillcompiling@gmail.com>
cyclonespi loads fpga firmware over spi, using the "passive serial"
interface on Altera Cyclone FPGAS.
one of the simpler ways to set up an fpga at runtime.
The signal interface is close to unidirectional spi with lsb first.
Signed-off-by: Joshua Clayton <stillcompiling@gmail.com>
---
drivers/fpga/Kconfig | 6 ++
drivers/fpga/Makefile | 1 +
drivers/fpga/cyclonespi.c | 173 ++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 180 insertions(+)
create mode 100644 drivers/fpga/cyclonespi.c
diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
index cd84934..ccad5b1 100644
--- a/drivers/fpga/Kconfig
+++ b/drivers/fpga/Kconfig
@@ -13,6 +13,12 @@ config FPGA
if FPGA
+config FPGA_MGR_CYCLONE_SPI
+ tristate "Altera Cyclone V SPI"
+ depends on SPI
+ help
+ FPGA manager driver support for Altera Cyclone V over SPI
+
config FPGA_MGR_SOCFPGA
tristate "Altera SOCFPGA FPGA Manager"
depends on ARCH_SOCFPGA
diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
index 8d83fc6..c03f40de 100644
--- a/drivers/fpga/Makefile
+++ b/drivers/fpga/Makefile
@@ -6,5 +6,6 @@
obj-$(CONFIG_FPGA) += fpga-mgr.o
# FPGA Manager Drivers
+obj-$(CONFIG_FPGA_MGR_CYCLONE_SPI) += cyclonespi.o
obj-$(CONFIG_FPGA_MGR_SOCFPGA) += socfpga.o
obj-$(CONFIG_FPGA_MGR_ZYNQ_FPGA) += zynq-fpga.o
diff --git a/drivers/fpga/cyclonespi.c b/drivers/fpga/cyclonespi.c
new file mode 100644
index 0000000..1ffa67c
--- /dev/null
+++ b/drivers/fpga/cyclonespi.c
@@ -0,0 +1,173 @@
+/**
+ * Copyright (c) 2015 United Western Technologies, Corporation
+ *
+ * Joshua Clayton <stillcompiling@gmail.com>
+ *
+ * Manage Altera fpga firmware that is loaded over spi.
+ * Firmware must be in binary "rbf" format.
+ * Works on Cyclone V. Should work on cyclone series.
+ * May work on other Altera fpgas.
+ *
+ */
+
+#include <linux/delay.h>
+#include <linux/fpga/fpga-mgr.h>
+#include <linux/gpio/consumer.h>
+#include <linux/module.h>
+#include <linux/of_gpio.h>
+#include <linux/spi/spi.h>
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Joshua Clayton <stillcompiling@gmail.com>");
+MODULE_DESCRIPTION("Module to load Altera FPGA firmware over spi");
+
+struct cyclonespi_conf {
+ struct gpio_desc *reset;
+ struct gpio_desc *status;
+ struct spi_device *spi;
+};
+
+static const struct of_device_id of_ef_match[] = {
+ { .compatible = "altr,cyclonespi-fpga-mgr", },
+ {}
+};
+MODULE_DEVICE_TABLE(of, of_ef_match);
+
+static enum fpga_mgr_states cyclonespi_state(struct fpga_manager *mgr)
+{
+ return mgr->state;
+}
+
+static inline u32 revbit8x4(u32 n)
+{
+ n = ((n & 0xF0F0F0F0UL) >> 4) | ((n & 0x0F0F0F0FUL) << 4);
+ n = ((n & 0xCCCCCCCCUL) >> 2) | ((n & 0x33333333UL) << 2);
+ n = ((n & 0xAAAAAAAAUL) >> 1) | ((n & 0x55555555UL) << 1);
+ return n;
+}
+
+static int cyclonespi_write_init(struct fpga_manager *mgr, u32 flags,
+ const char *buf, size_t count)
+{
+ struct cyclonespi_conf *conf = (struct cyclonespi_conf *)mgr->priv;
+ u32 *fw32 = (u32 *)buf;
+ const u32 *fw_end = (u32 *)(buf + count);
+
+ if (flags & FPGA_MGR_PARTIAL_RECONFIG) {
+ dev_err(&mgr->dev, "Partial reconfiguration not supported.\n");
+ return -EINVAL;
+ }
+
+ gpiod_set_value(conf->reset, 0);
+ udelay(50);
+ if (gpiod_get_value(conf->status) == 1) {
+ dev_err(&mgr->dev, "Status pin should be low.\n");
+ return -EIO;
+ }
+
+ gpiod_set_value(conf->reset, 1);
+ msleep(1);
+ if (gpiod_get_value(conf->status) == 0) {
+ dev_err(&mgr->dev, "Status pin not ready.\n");
+ return -EIO;
+ }
+
+ /* set buffer to lsb first */
+ while (fw32 < fw_end) {
+ *fw32 = revbit8x4(*fw32);
+ fw32++;
+ }
+
+ return 0;
+}
+
+static int cyclonespi_write(struct fpga_manager *mgr, const char *buf,
+ size_t count)
+{
+ struct cyclonespi_conf *conf = (struct cyclonespi_conf *)mgr->priv;
+ const char *fw_data = buf;
+ const char *fw_data_end = fw_data + count;
+
+ while (fw_data < fw_data_end) {
+ int ret;
+ int stride = fw_data_end - fw_data;
+
+ if (stride > SZ_4K)
+ stride = SZ_4K;
+
+ ret = spi_write(conf->spi, fw_data, stride);
+ if (ret) {
+ dev_err(&mgr->dev, "spi error in firmware write: %d\n",
+ ret);
+ return ret;
+ }
+ fw_data += stride;
+ }
+
+ return 0;
+}
+
+static int cyclonespi_write_complete(struct fpga_manager *mgr, u32 flags)
+{
+ struct cyclonespi_conf *conf = (struct cyclonespi_conf *)mgr->priv;
+
+ if (gpiod_get_value(conf->status) == 0) {
+ dev_err(&mgr->dev, "Error during configuration.\n");
+ return -EIO;
+ }
+
+ return 0;
+}
+
+static const struct fpga_manager_ops cyclonespi_ops = {
+ .state = cyclonespi_state,
+ .write_init = cyclonespi_write_init,
+ .write = cyclonespi_write,
+ .write_complete = cyclonespi_write_complete,
+};
+
+static int cyclonespi_probe(struct spi_device *spi)
+{
+ struct cyclonespi_conf *conf = devm_kzalloc(&spi->dev, sizeof(*conf),
+ GFP_KERNEL);
+
+ if (!conf)
+ return -ENOMEM;
+
+ conf->spi = spi;
+ conf->reset = devm_gpiod_get(&spi->dev, "reset", GPIOD_OUT_LOW);
+ if (IS_ERR(conf->reset)) {
+ dev_err(&spi->dev, "Failed to get reset gpio: %ld\n",
+ PTR_ERR(conf->reset));
+ return PTR_ERR(conf->reset);
+ }
+
+ conf->status = devm_gpiod_get(&spi->dev, "status", GPIOD_IN);
+ if (IS_ERR(conf->status)) {
+ dev_err(&spi->dev, "Failed to get status gpio: %ld\n",
+ PTR_ERR(conf->status));
+ return PTR_ERR(conf->status);
+ }
+
+ return fpga_mgr_register(&spi->dev, "Altera SPI FPGA Manager",
+ &cyclonespi_ops, conf);
+}
+
+static int cyclonespi_remove(struct spi_device *spi)
+{
+ fpga_mgr_unregister(&spi->dev);
+
+ return 0;
+}
+
+static struct spi_driver cyclonespi_driver = {
+ .driver = {
+ .name = "cyclonespi",
+ .owner = THIS_MODULE,
+ .of_match_table = of_match_ptr(of_ef_match),
+ },
+ .probe = cyclonespi_probe,
+ .remove = cyclonespi_remove,
+};
+
+module_spi_driver(cyclonespi_driver)
--
2.7.4
^ permalink raw reply related
* [PATCH v13 15/15] vfio/type1: Return the MSI geometry through VFIO_IOMMU_GET_INFO capability chains
From: Alex Williamson @ 2016-10-06 20:20 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1475743531-4780-16-git-send-email-eric.auger@redhat.com>
On Thu, 6 Oct 2016 08:45:31 +0000
Eric Auger <eric.auger@redhat.com> wrote:
> This patch allows the user-space to retrieve the MSI geometry. The
> implementation is based on capability chains, now also added to
> VFIO_IOMMU_GET_INFO.
>
> The returned info comprise:
> - whether the MSI IOVA are constrained to a reserved range (x86 case) and
> in the positive, the start/end of the aperture,
> - or whether the IOVA aperture need to be set by the userspace. In that
> case, the size and alignment of the IOVA window to be provided are
> returned.
>
> In case the userspace must provide the IOVA aperture, we currently report
> a size/alignment based on all the doorbells registered by the host kernel.
> This may exceed the actual needs.
>
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>
> ---
> v11 -> v11:
> - msi_doorbell_pages was renamed msi_doorbell_calc_pages
>
> v9 -> v10:
> - move cap_offset after iova_pgsizes
> - replace __u64 alignment by __u32 order
> - introduce __u32 flags in vfio_iommu_type1_info_cap_msi_geometry and
> fix alignment
> - call msi-doorbell API to compute the size/alignment
>
> v8 -> v9:
> - use iommu_msi_supported flag instead of programmable
> - replace IOMMU_INFO_REQUIRE_MSI_MAP flag by a more sophisticated
> capability chain, reporting the MSI geometry
>
> v7 -> v8:
> - use iommu_domain_msi_geometry
>
> v6 -> v7:
> - remove the computation of the number of IOVA pages to be provisionned.
> This number depends on the domain/group/device topology which can
> dynamically change. Let's rely instead rely on an arbitrary max depending
> on the system
>
> v4 -> v5:
> - move msi_info and ret declaration within the conditional code
>
> v3 -> v4:
> - replace former vfio_domains_require_msi_mapping by
> more complex computation of MSI mapping requirements, especially the
> number of pages to be provided by the user-space.
> - reword patch title
>
> RFC v1 -> v1:
> - derived from
> [RFC PATCH 3/6] vfio: Extend iommu-info to return MSIs automap state
> - renamed allow_msi_reconfig into require_msi_mapping
> - fixed VFIO_IOMMU_GET_INFO
> ---
> drivers/vfio/vfio_iommu_type1.c | 78 ++++++++++++++++++++++++++++++++++++++++-
> include/uapi/linux/vfio.h | 32 ++++++++++++++++-
> 2 files changed, 108 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
> index dc3ee5d..ce5e7eb 100644
> --- a/drivers/vfio/vfio_iommu_type1.c
> +++ b/drivers/vfio/vfio_iommu_type1.c
> @@ -38,6 +38,8 @@
> #include <linux/workqueue.h>
> #include <linux/dma-iommu.h>
> #include <linux/msi-doorbell.h>
> +#include <linux/irqdomain.h>
> +#include <linux/msi.h>
>
> #define DRIVER_VERSION "0.2"
> #define DRIVER_AUTHOR "Alex Williamson <alex.williamson@redhat.com>"
> @@ -1101,6 +1103,55 @@ static int vfio_domains_have_iommu_cache(struct vfio_iommu *iommu)
> return ret;
> }
>
> +static int compute_msi_geometry_caps(struct vfio_iommu *iommu,
> + struct vfio_info_cap *caps)
> +{
> + struct vfio_iommu_type1_info_cap_msi_geometry *vfio_msi_geometry;
> + unsigned long order = __ffs(vfio_pgsize_bitmap(iommu));
> + struct iommu_domain_msi_geometry msi_geometry;
> + struct vfio_info_cap_header *header;
> + struct vfio_domain *d;
> + bool reserved;
> + size_t size;
> +
> + mutex_lock(&iommu->lock);
> + /* All domains have same require_msi_map property, pick first */
> + d = list_first_entry(&iommu->domain_list, struct vfio_domain, next);
> + iommu_domain_get_attr(d->domain, DOMAIN_ATTR_MSI_GEOMETRY,
> + &msi_geometry);
> + reserved = !msi_geometry.iommu_msi_supported;
> +
> + mutex_unlock(&iommu->lock);
> +
> + size = sizeof(*vfio_msi_geometry);
> + header = vfio_info_cap_add(caps, size,
> + VFIO_IOMMU_TYPE1_INFO_CAP_MSI_GEOMETRY, 1);
> +
> + if (IS_ERR(header))
> + return PTR_ERR(header);
> +
> + vfio_msi_geometry = container_of(header,
> + struct vfio_iommu_type1_info_cap_msi_geometry,
> + header);
> +
> + vfio_msi_geometry->flags = reserved;
Use the bit flag VFIO_IOMMU_MSI_GEOMETRY_RESERVED
> + if (reserved) {
> + vfio_msi_geometry->aperture_start = msi_geometry.aperture_start;
> + vfio_msi_geometry->aperture_end = msi_geometry.aperture_end;
But maybe nobody has set these, did you intend to use
iommu_domain_msi_aperture_valid(), which you defined early on but never
used?
> + return 0;
> + }
> +
> + vfio_msi_geometry->order = order;
I'm tempted to suggest that a user could do the same math on their own
since we provide the supported bitmap already... could it ever not be
the same?
> + /*
> + * we compute a system-wide requirement based on all the registered
> + * doorbells
> + */
> + vfio_msi_geometry->size =
> + msi_doorbell_calc_pages(order) * ((uint64_t) 1 << order);
> +
> + return 0;
> +}
> +
> static long vfio_iommu_type1_ioctl(void *iommu_data,
> unsigned int cmd, unsigned long arg)
> {
> @@ -1122,8 +1173,10 @@ static long vfio_iommu_type1_ioctl(void *iommu_data,
> }
> } else if (cmd == VFIO_IOMMU_GET_INFO) {
> struct vfio_iommu_type1_info info;
> + struct vfio_info_cap caps = { .buf = NULL, .size = 0 };
> + int ret;
>
> - minsz = offsetofend(struct vfio_iommu_type1_info, iova_pgsizes);
> + minsz = offsetofend(struct vfio_iommu_type1_info, cap_offset);
>
> if (copy_from_user(&info, (void __user *)arg, minsz))
> return -EFAULT;
> @@ -1135,6 +1188,29 @@ static long vfio_iommu_type1_ioctl(void *iommu_data,
>
> info.iova_pgsizes = vfio_pgsize_bitmap(iommu);
>
> + ret = compute_msi_geometry_caps(iommu, &caps);
> + if (ret)
> + return ret;
> +
> + if (caps.size) {
> + info.flags |= VFIO_IOMMU_INFO_CAPS;
> + if (info.argsz < sizeof(info) + caps.size) {
> + info.argsz = sizeof(info) + caps.size;
> + info.cap_offset = 0;
> + } else {
> + vfio_info_cap_shift(&caps, sizeof(info));
> + if (copy_to_user((void __user *)arg +
> + sizeof(info), caps.buf,
> + caps.size)) {
> + kfree(caps.buf);
> + return -EFAULT;
> + }
> + info.cap_offset = sizeof(info);
> + }
> +
> + kfree(caps.buf);
> + }
> +
> return copy_to_user((void __user *)arg, &info, minsz) ?
> -EFAULT : 0;
>
> diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
> index 4a9dbc2..8dae013 100644
> --- a/include/uapi/linux/vfio.h
> +++ b/include/uapi/linux/vfio.h
> @@ -488,7 +488,35 @@ struct vfio_iommu_type1_info {
> __u32 argsz;
> __u32 flags;
> #define VFIO_IOMMU_INFO_PGSIZES (1 << 0) /* supported page sizes info */
> - __u64 iova_pgsizes; /* Bitmap of supported page sizes */
> +#define VFIO_IOMMU_INFO_CAPS (1 << 1) /* Info supports caps */
> + __u64 iova_pgsizes; /* Bitmap of supported page sizes */
> + __u32 __resv;
> + __u32 cap_offset; /* Offset within info struct of first cap */
> +};
I understand the padding, but not the ordering. Why not end with
padding?
> +
> +#define VFIO_IOMMU_TYPE1_INFO_CAP_MSI_GEOMETRY 1
> +
> +/*
> + * The MSI geometry capability allows to report the MSI IOVA geometry:
> + * - either the MSI IOVAs are constrained within a reserved IOVA aperture
> + * whose boundaries are given by [@aperture_start, @aperture_end].
> + * this is typically the case on x86 host. The userspace is not allowed
> + * to map userspace memory at IOVAs intersecting this range using
> + * VFIO_IOMMU_MAP_DMA.
> + * - or the MSI IOVAs are not requested to belong to any reserved range;
> + * in that case the userspace must provide an IOVA window characterized by
> + * @size and @alignment using VFIO_IOMMU_MAP_DMA with RESERVED_MSI_IOVA flag.
> + */
> +struct vfio_iommu_type1_info_cap_msi_geometry {
> + struct vfio_info_cap_header header;
> + __u32 flags;
> +#define VFIO_IOMMU_MSI_GEOMETRY_RESERVED (1 << 0) /* reserved geometry */
> + /* not reserved */
> + __u32 order; /* iommu page order used for aperture alignment*/
> + __u64 size; /* IOVA aperture size (bytes) the userspace must provide */
> + /* reserved */
> + __u64 aperture_start;
> + __u64 aperture_end;
Should these be a union? We never set them both. Should the !reserved
case have a flag as well, so the user can positively identify what's
being provided?
> };
>
> #define VFIO_IOMMU_GET_INFO _IO(VFIO_TYPE, VFIO_BASE + 12)
> @@ -503,6 +531,8 @@ struct vfio_iommu_type1_info {
> * IOVA region that will be used on some platforms to map the host MSI frames.
> * In that specific case, vaddr is ignored. Once registered, an MSI reserved
> * IOVA region stays until the container is closed.
> + * The requirement for provisioning such reserved IOVA range can be checked by
> + * checking the VFIO_IOMMU_TYPE1_INFO_CAP_MSI_GEOMETRY capability.
> */
> struct vfio_iommu_type1_dma_map {
> __u32 argsz;
^ permalink raw reply
* [PATCH v13 13/15] vfio/type1: Check doorbell safety
From: Alex Williamson @ 2016-10-06 20:19 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1475743531-4780-14-git-send-email-eric.auger@redhat.com>
On Thu, 6 Oct 2016 08:45:29 +0000
Eric Auger <eric.auger@redhat.com> wrote:
> On x86 IRQ remapping is abstracted by the IOMMU. On ARM this is abstracted
> by the msi controller.
>
> Since we currently have no way to detect whether the MSI controller is
> upstream or downstream to the IOMMU we rely on the MSI doorbell information
> registered by the interrupt controllers. In case at least one doorbell
> does not implement proper isolation, we state the assignment is unsafe
> with regard to interrupts. This is a coase assessment but should allow to
> wait for a better system description.
s/coase/coarse/
>
> At this point ARM sMMU still advertises IOMMU_CAP_INTR_REMAP. This is
> removed in next patch.
>
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>
> ---
>
> v9 -> v10:
> - coarse safety assessment based on MSI doorbell info
>
> v3 -> v4:
> - rename vfio_msi_parent_irq_remapping_capable into vfio_safe_irq_domain
> and irq_remapping into safe_irq_domains
>
> v2 -> v3:
> - protect vfio_msi_parent_irq_remapping_capable with
> CONFIG_GENERIC_MSI_IRQ_DOMAIN
> ---
> drivers/vfio/vfio_iommu_type1.c | 8 +++++++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
> index c2f8bd9..dc3ee5d 100644
> --- a/drivers/vfio/vfio_iommu_type1.c
> +++ b/drivers/vfio/vfio_iommu_type1.c
> @@ -37,6 +37,7 @@
> #include <linux/vfio.h>
> #include <linux/workqueue.h>
> #include <linux/dma-iommu.h>
> +#include <linux/msi-doorbell.h>
>
> #define DRIVER_VERSION "0.2"
> #define DRIVER_AUTHOR "Alex Williamson <alex.williamson@redhat.com>"
> @@ -921,8 +922,13 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
> INIT_LIST_HEAD(&domain->group_list);
> list_add(&group->next, &domain->group_list);
>
> + /*
> + * to advertise safe interrupts either the IOMMU or the MSI controllers
> + * must support IRQ remapping (aka. interrupt translation)
> + */
> if (!allow_unsafe_interrupts &&
> - !iommu_capable(bus, IOMMU_CAP_INTR_REMAP)) {
> + (!iommu_capable(bus, IOMMU_CAP_INTR_REMAP) &&
> + !msi_doorbell_safe())) {
I assume this is why you want msi_doorbell_safe() to return true when
!CONFIG_MSI_DOORBELL but don't we really want to look at the iommu
geometry to see if MSI mapping is supported and then, once we know the
iommu is participating in MSI mapping, whether it's safe?
> pr_warn("%s: No interrupt remapping support. Use the module param \"allow_unsafe_interrupts\" to enable VFIO IOMMU support on this platform\n",
> __func__);
> ret = -EPERM;
^ permalink raw reply
* [PATCH v13 12/15] vfio: Allow reserved msi iova registration
From: Alex Williamson @ 2016-10-06 20:19 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1475743531-4780-13-git-send-email-eric.auger@redhat.com>
On Thu, 6 Oct 2016 08:45:28 +0000
Eric Auger <eric.auger@redhat.com> wrote:
> The user is allowed to register a reserved MSI IOVA range by using the
> DMA MAP API and setting the new flag: VFIO_DMA_MAP_FLAG_MSI_RESERVED_IOVA.
> This region is stored in the vfio_dma rb tree. At that point the iova
> range is not mapped to any target address yet. The host kernel will use
> those iova when needed, typically when MSIs are allocated.
>
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> Signed-off-by: Bharat Bhushan <Bharat.Bhushan@freescale.com>
>
> ---
> v12 -> v13:
> - use iommu_get_dma_msi_region_cookie
>
> v9 -> v10
> - use VFIO_IOVA_RESERVED_MSI enum value
>
> v7 -> v8:
> - use iommu_msi_set_aperture function. There is no notion of
> unregistration anymore since the reserved msi slot remains
> until the container gets closed.
>
> v6 -> v7:
> - use iommu_free_reserved_iova_domain
> - convey prot attributes downto dma-reserved-iommu iova domain creation
> - reserved bindings teardown now performed on iommu domain destruction
> - rename VFIO_DMA_MAP_FLAG_MSI_RESERVED_IOVA into
> VFIO_DMA_MAP_FLAG_RESERVED_MSI_IOVA
> - change title
> - pass the protection attribute to dma-reserved-iommu API
>
> v3 -> v4:
> - use iommu_alloc/free_reserved_iova_domain exported by dma-reserved-iommu
> - protect vfio_register_reserved_iova_range implementation with
> CONFIG_IOMMU_DMA_RESERVED
> - handle unregistration by user-space and on vfio_iommu_type1 release
>
> v1 -> v2:
> - set returned value according to alloc_reserved_iova_domain result
> - free the iova domains in case any error occurs
>
> RFC v1 -> v1:
> - takes into account Alex comments, based on
> [RFC PATCH 1/6] vfio: Add interface for add/del reserved iova region:
> - use the existing dma map/unmap ioctl interface with a flag to register
> a reserved IOVA range. A single reserved iova region is allowed.
> ---
> drivers/vfio/vfio_iommu_type1.c | 77 ++++++++++++++++++++++++++++++++++++++++-
> include/uapi/linux/vfio.h | 10 +++++-
> 2 files changed, 85 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
> index 5bc5fc9..c2f8bd9 100644
> --- a/drivers/vfio/vfio_iommu_type1.c
> +++ b/drivers/vfio/vfio_iommu_type1.c
> @@ -442,6 +442,20 @@ static void vfio_unmap_unpin(struct vfio_iommu *iommu, struct vfio_dma *dma)
> vfio_lock_acct(-unlocked);
> }
>
> +static int vfio_set_msi_aperture(struct vfio_iommu *iommu,
> + dma_addr_t iova, size_t size)
> +{
> + struct vfio_domain *d;
> + int ret = 0;
> +
> + list_for_each_entry(d, &iommu->domain_list, next) {
> + ret = iommu_get_dma_msi_region_cookie(d->domain, iova, size);
> + if (ret)
> + break;
> + }
> + return ret;
Doesn't this need an unwind on failure loop?
> +}
> +
> static void vfio_remove_dma(struct vfio_iommu *iommu, struct vfio_dma *dma)
> {
> vfio_unmap_unpin(iommu, dma);
> @@ -691,6 +705,63 @@ static int vfio_dma_do_map(struct vfio_iommu *iommu,
> return ret;
> }
>
> +static int vfio_register_msi_range(struct vfio_iommu *iommu,
> + struct vfio_iommu_type1_dma_map *map)
> +{
> + dma_addr_t iova = map->iova;
> + size_t size = map->size;
> + int ret = 0;
> + struct vfio_dma *dma;
> + unsigned long order;
> + uint64_t mask;
> +
> + /* Verify that none of our __u64 fields overflow */
> + if (map->size != size || map->iova != iova)
> + return -EINVAL;
> +
> + order = __ffs(vfio_pgsize_bitmap(iommu));
> + mask = ((uint64_t)1 << order) - 1;
> +
> + WARN_ON(mask & PAGE_MASK);
> +
> + if (!size || (size | iova) & mask)
> + return -EINVAL;
> +
> + /* Don't allow IOVA address wrap */
> + if (iova + size - 1 < iova)
> + return -EINVAL;
> +
> + mutex_lock(&iommu->lock);
> +
> + if (vfio_find_dma(iommu, iova, size, VFIO_IOVA_ANY)) {
> + ret = -EEXIST;
> + goto unlock;
> + }
> +
> + dma = kzalloc(sizeof(*dma), GFP_KERNEL);
> + if (!dma) {
> + ret = -ENOMEM;
> + goto unlock;
> + }
> +
> + dma->iova = iova;
> + dma->size = size;
> + dma->type = VFIO_IOVA_RESERVED_MSI;
> +
> + ret = vfio_set_msi_aperture(iommu, iova, size);
> + if (ret)
> + goto free_unlock;
> +
> + vfio_link_dma(iommu, dma);
> + goto unlock;
> +
> +free_unlock:
> + kfree(dma);
> +unlock:
> + mutex_unlock(&iommu->lock);
> + return ret;
> +}
> +
> static int vfio_bus_type(struct device *dev, void *data)
> {
> struct bus_type **bus = data;
> @@ -1064,7 +1135,8 @@ static long vfio_iommu_type1_ioctl(void *iommu_data,
> } else if (cmd == VFIO_IOMMU_MAP_DMA) {
> struct vfio_iommu_type1_dma_map map;
> uint32_t mask = VFIO_DMA_MAP_FLAG_READ |
> - VFIO_DMA_MAP_FLAG_WRITE;
> + VFIO_DMA_MAP_FLAG_WRITE |
> + VFIO_DMA_MAP_FLAG_RESERVED_MSI_IOVA;
>
> minsz = offsetofend(struct vfio_iommu_type1_dma_map, size);
>
> @@ -1074,6 +1146,9 @@ static long vfio_iommu_type1_ioctl(void *iommu_data,
> if (map.argsz < minsz || map.flags & ~mask)
> return -EINVAL;
>
> + if (map.flags & VFIO_DMA_MAP_FLAG_RESERVED_MSI_IOVA)
> + return vfio_register_msi_range(iommu, &map);
> +
> return vfio_dma_do_map(iommu, &map);
>
> } else if (cmd == VFIO_IOMMU_UNMAP_DMA) {
> diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
> index 255a211..4a9dbc2 100644
> --- a/include/uapi/linux/vfio.h
> +++ b/include/uapi/linux/vfio.h
> @@ -498,12 +498,19 @@ struct vfio_iommu_type1_info {
> *
> * Map process virtual addresses to IO virtual addresses using the
> * provided struct vfio_dma_map. Caller sets argsz. READ &/ WRITE required.
> + *
> + * In case RESERVED_MSI_IOVA flag is set, the API only aims at registering an
> + * IOVA region that will be used on some platforms to map the host MSI frames.
> + * In that specific case, vaddr is ignored. Once registered, an MSI reserved
> + * IOVA region stays until the container is closed.
> */
> struct vfio_iommu_type1_dma_map {
> __u32 argsz;
> __u32 flags;
> #define VFIO_DMA_MAP_FLAG_READ (1 << 0) /* readable from device */
> #define VFIO_DMA_MAP_FLAG_WRITE (1 << 1) /* writable from device */
> +/* reserved iova for MSI vectors*/
> +#define VFIO_DMA_MAP_FLAG_RESERVED_MSI_IOVA (1 << 2)
> __u64 vaddr; /* Process virtual address */
> __u64 iova; /* IO virtual address */
> __u64 size; /* Size of mapping (bytes) */
> @@ -519,7 +526,8 @@ struct vfio_iommu_type1_dma_map {
> * Caller sets argsz. The actual unmapped size is returned in the size
> * field. No guarantee is made to the user that arbitrary unmaps of iova
> * or size different from those used in the original mapping call will
> - * succeed.
> + * succeed. Once registered, an MSI region cannot be unmapped and stays
> + * until the container is closed.
> */
> struct vfio_iommu_type1_dma_unmap {
> __u32 argsz;
What happens when an x86 user does a mapping with this new flag set?
It seems like we end up configuring everything just as we would on a
platform requiring MSI mapping, including setting the domain MSI
geometry. Should we be testing the MSI geometry flag on the iommu to
see if this is supported? Surprisingly few things seem to check that
flag.
^ permalink raw reply
* [PATCH v13 11/15] vfio/type1: Handle unmap/unpin and replay for VFIO_IOVA_RESERVED slots
From: Alex Williamson @ 2016-10-06 20:19 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1475743531-4780-12-git-send-email-eric.auger@redhat.com>
On Thu, 6 Oct 2016 08:45:27 +0000
Eric Auger <eric.auger@redhat.com> wrote:
> Before allowing the end-user to create VFIO_IOVA_RESERVED dma slots,
> let's implement the expected behavior for removal and replay.
>
> As opposed to user dma slots, reserved IOVAs are not systematically bound
> to PAs and PAs are not pinned. VFIO just initializes the IOVA "aperture".
> IOVAs are allocated outside of the VFIO framework, by the MSI layer which
> is responsible to free and unmap them. The MSI mapping resources are freeed
nit, extra 'e', "freed"
> by the IOMMU driver on domain destruction.
>
> On the creation of a new domain, the "replay" of a reserved slot simply
> needs to set the MSI aperture on the new domain.
>
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>
> ---
> v12 -> v13:
> - use dma-iommu iommu_get_dma_msi_region_cookie
>
> v9 -> v10:
> - replay of a reserved slot sets the MSI aperture on the new domain
> - use VFIO_IOVA_RESERVED_MSI enum value instead of VFIO_IOVA_RESERVED
>
> v7 -> v8:
> - do no destroy anything anymore, just bypass unmap/unpin and iommu_map
> on replay
> ---
> drivers/vfio/Kconfig | 1 +
> drivers/vfio/vfio_iommu_type1.c | 10 +++++++++-
> 2 files changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/vfio/Kconfig b/drivers/vfio/Kconfig
> index da6e2ce..673ec79 100644
> --- a/drivers/vfio/Kconfig
> +++ b/drivers/vfio/Kconfig
> @@ -1,6 +1,7 @@
> config VFIO_IOMMU_TYPE1
> tristate
> depends on VFIO
> + select IOMMU_DMA
> default n
>
> config VFIO_IOMMU_SPAPR_TCE
> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
> index 65a4038..5bc5fc9 100644
> --- a/drivers/vfio/vfio_iommu_type1.c
> +++ b/drivers/vfio/vfio_iommu_type1.c
> @@ -36,6 +36,7 @@
> #include <linux/uaccess.h>
> #include <linux/vfio.h>
> #include <linux/workqueue.h>
> +#include <linux/dma-iommu.h>
>
> #define DRIVER_VERSION "0.2"
> #define DRIVER_AUTHOR "Alex Williamson <alex.williamson@redhat.com>"
> @@ -387,7 +388,7 @@ static void vfio_unmap_unpin(struct vfio_iommu *iommu, struct vfio_dma *dma)
> struct vfio_domain *domain, *d;
> long unlocked = 0;
>
> - if (!dma->size)
> + if (!dma->size || dma->type != VFIO_IOVA_USER)
> return;
> /*
> * We use the IOMMU to track the physical addresses, otherwise we'd
> @@ -724,6 +725,13 @@ static int vfio_iommu_replay(struct vfio_iommu *iommu,
> dma = rb_entry(n, struct vfio_dma, node);
> iova = dma->iova;
>
> + if (dma->type == VFIO_IOVA_RESERVED_MSI) {
> + ret = iommu_get_dma_msi_region_cookie(domain->domain,
> + dma->iova, dma->size);
> + WARN_ON(ret);
> + continue;
> + }
Why is this a passable error? We consider an iommu_map() error on any
entry a failure.
> +
> while (iova < dma->iova + dma->size) {
> phys_addr_t phys = iommu_iova_to_phys(d->domain, iova);
> size_t size;
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox