* RE: [PATCH] sata_fsl: add workaround for data length mismatch on freescale V2 controller
From: Jenkins, Clive @ 2012-09-04 12:00 UTC (permalink / raw)
To: Shaohui Xie, jgarzik, linux-ide; +Cc: linuxppc-dev, linux-kernel, Anju Bhartiya
In-Reply-To: <1346756920-19128-1-git-send-email-Shaohui.Xie@freescale.com>
> The freescale V2 SATA controller checks
> if the received data length matches
> the programmed length 'ttl', if not,
> it assumes that this is an error.
...
Can you tell us exactly what
"The freescale V2 SATA controller" is,
and what versions of what devices contain it?
Thanks,
Clive
^ permalink raw reply
* RE: [PATCH v7 1/8] Talitos: Support for async_tx XOR offload
From: Liu Qiang-B32616 @ 2012-09-04 12:28 UTC (permalink / raw)
To: Dan Williams
Cc: Li Yang-R58472, arnd@arndb.de, vinod.koul@intel.com,
gregkh@linuxfoundation.org, linux-kernel@vger.kernel.org,
Phillips Kim-R1AAHA, linux-crypto@vger.kernel.org,
dan.j.williams@intel.com, linuxppc-dev@lists.ozlabs.org,
davem@davemloft.net, herbert@gondor.apana.org.au
In-Reply-To: <CAA9_cmdqmuhAcaqfkjvKSpsTL350BAJcY8pn6U=SJ9Nok1-XuA@mail.gmail.com>
> -----Original Message-----
> From: dan.j.williams@gmail.com [mailto:dan.j.williams@gmail.com] On
> Behalf Of Dan Williams
> Sent: Sunday, September 02, 2012 4:12 PM
> To: Liu Qiang-B32616
> Cc: linux-crypto@vger.kernel.org; herbert@gondor.hengli.com.au;
> davem@davemloft.net; linux-kernel@vger.kernel.org; linuxppc-
> dev@lists.ozlabs.org; Li Yang-R58472; Phillips Kim-R1AAHA;
> vinod.koul@intel.com; dan.j.williams@intel.com; arnd@arndb.de;
> gregkh@linuxfoundation.org
> Subject: Re: [PATCH v7 1/8] Talitos: Support for async_tx XOR offload
>=20
> On Thu, Aug 9, 2012 at 1:20 AM, <qiang.liu@freescale.com> wrote:
> > From: Qiang Liu <qiang.liu@freescale.com>
> >
> > Expose Talitos's XOR functionality to be used for RAID parity
> > calculation via the Async_tx layer.
> >
> > Cc: Herbert Xu <herbert@gondor.apana.org.au>
> > Cc: David S. Miller <davem@davemloft.net>
> > Signed-off-by: Dipen Dudhat <Dipen.Dudhat@freescale.com>
> > Signed-off-by: Maneesh Gupta <Maneesh.Gupta@freescale.com>
> > Signed-off-by: Kim Phillips <kim.phillips@freescale.com>
> > Signed-off-by: Vishnu Suresh <Vishnu@freescale.com>
> > Signed-off-by: Qiang Liu <qiang.liu@freescale.com>
> > ---
> > drivers/crypto/Kconfig | 9 +
> > drivers/crypto/talitos.c | 413
> ++++++++++++++++++++++++++++++++++++++++++++++
> > drivers/crypto/talitos.h | 53 ++++++
> > 3 files changed, 475 insertions(+), 0 deletions(-)
> >
> > diff --git a/drivers/crypto/Kconfig b/drivers/crypto/Kconfig
> > index be6b2ba..f0a7c29 100644
> > --- a/drivers/crypto/Kconfig
> > +++ b/drivers/crypto/Kconfig
> > @@ -222,6 +222,15 @@ config CRYPTO_DEV_TALITOS
> > To compile this driver as a module, choose M here: the module
> > will be called talitos.
> >
> > +config CRYPTO_DEV_TALITOS_RAIDXOR
> > + bool "Talitos RAID5 XOR Calculation Offload"
> > + default y
>=20
> No, default y. The user should explicitly enable this.
Ok, I will change it to N.
>=20
> > + select DMA_ENGINE
> > + depends on CRYPTO_DEV_TALITOS
> > + help
> > + Say 'Y' here to use the Freescale Security Engine (SEC) to
> > + offload RAID XOR parity Calculation
> > +
>=20
> Is it faster than cpu xor?
Yes, I tested with IOZone, cpu load is also reduced.
I think one possible reason is the processor of p1022ds is not strong as ot=
hers, so the performance is improved obviously.
>=20
> Will this engine be coordinating with another to handle memory copies?
> The dma mapping code for async_tx/raid is broken when dma mapping
> requests overlap or cross dma device boundaries [1].
>=20
> [1]: http://marc.info/?l=3Dlinux-arm-kernel&m=3D129407269402930&w=3D2
Yes, it needs fsl-dma to handle memcpy copies.
I read your link, the unmap address is stored in talitos hwdesc, the addres=
s will be unmapped when async_tx ack this descriptor, I know fsl-dma won't =
wait this ack flag in current kernel, so I fix it in fsl-dma patch 5/8. Do =
you mean that?
>=20
> > +static void talitos_process_pending(struct talitos_xor_chan *xor_chan)
> > +{
> > + struct talitos_xor_desc *desc, *_desc;
> > + unsigned long flags;
> > + int status;
> > + struct talitos_private *priv;
> > + int ch;
> > +
> > + priv =3D dev_get_drvdata(xor_chan->dev);
> > + ch =3D atomic_inc_return(&priv->last_chan) &
> > + (priv->num_channels - 1);
>=20
> Maybe a comment about why this this is duplicated from
> talitos_cra_init()? It sticks out here and I had to go looking to
> find out why this channel number increments on submit.
My fault, I will correct it as below,
atomic_read((&priv->last_chan) & (priv->num_channels - 1);
>=20
>=20
> > + spin_lock_irqsave(&xor_chan->desc_lock, flags);
> > +
> > + list_for_each_entry_safe(desc, _desc, &xor_chan->pending_q,
> node) {
> > + status =3D talitos_submit(xor_chan->dev, ch, &desc-
> >hwdesc,
> > + talitos_release_xor, desc);
> > + if (status !=3D -EINPROGRESS)
> > + break;
> > +
> > + list_del(&desc->node);
> > + list_add_tail(&desc->node, &xor_chan->in_progress_q);
> > + }
> > +
> > + spin_unlock_irqrestore(&xor_chan->desc_lock, flags);
> > +}
> > +
> > +static void talitos_xor_run_tx_complete_actions(struct
> talitos_xor_desc *desc,
> > + struct talitos_xor_chan *xor_chan)
> > +{
> > + struct device *dev =3D xor_chan->dev;
> > + dma_addr_t dest, addr;
> > + unsigned int src_cnt =3D desc->unmap_src_cnt;
> > + unsigned int len =3D desc->unmap_len;
> > + enum dma_ctrl_flags flags =3D desc->async_tx.flags;
> > + struct dma_async_tx_descriptor *tx =3D &desc->async_tx;
> > +
> > + /* unmap dma addresses */
> > + dest =3D desc->hwdesc.ptr[6].ptr;
> > + if (likely(!(flags & DMA_COMPL_SKIP_DEST_UNMAP)))
> > + dma_unmap_page(dev, dest, len, DMA_BIDIRECTIONAL);
> > +
> > + desc->idx =3D 6 - src_cnt;
> > + if (likely(!(flags & DMA_COMPL_SKIP_SRC_UNMAP))) {
> > + while(desc->idx < 6) {
>=20
> Checkpatch says:
> ERROR: space required before the open parenthesis '('
I will correct it next.
>=20
>=20
> > + addr =3D desc->hwdesc.ptr[desc->idx++].ptr;
> > + if (addr =3D=3D dest)
> > + continue;
> > + dma_unmap_page(dev, addr, len, DMA_TO_DEVICE);
> > + }
> > + }
> > +
> > + /* run dependent operations */
> > + dma_run_dependencies(tx);
>=20
> Here is where we run into problems if another engine accesses these
> same buffers, especially on ARM v6+.
Do you mean in dma_run_dependencies() will occur to the async_tx descriptor=
of fsl-dma will be processed? I'm not clearly.
>=20
> > +}
> > +
> > +static void talitos_release_xor(struct device *dev, struct
> talitos_desc *hwdesc,
> > + void *context, int error)
> > +{
> > + struct talitos_xor_desc *desc =3D context;
> > + struct talitos_xor_chan *xor_chan;
> > + dma_async_tx_callback callback;
> > + void *callback_param;
> > +
> > + if (unlikely(error))
> > + dev_err(dev, "xor operation: talitos error %d\n",
> error);
> > +
> > + xor_chan =3D container_of(desc->async_tx.chan, struct
> talitos_xor_chan,
> > + common);
> > + spin_lock_bh(&xor_chan->desc_lock);
> > + if (xor_chan->completed_cookie < desc->async_tx.cookie)
> > + xor_chan->completed_cookie =3D desc->async_tx.cookie;
> > +
>=20
> Use dma_cookie_complete().
Ok, I will correct it.
>=20
> > + callback =3D desc->async_tx.callback;
> > + callback_param =3D desc->async_tx.callback_param;
> > +
> > + if (callback) {
> > + spin_unlock_bh(&xor_chan->desc_lock);
> > + callback(callback_param);
> > + spin_lock_bh(&xor_chan->desc_lock);
>=20
> As mentioned you'll either need to ensure that
> talitos_process_pending() is only called from the tasklet, or upgrade
> these locks to hardirq safe.
The point is flush_channel can be called both of interrupt and tasklet.
So the process should be redesigned that make sure talitos_process_pending(=
) is only called from tasklet. I will fix it in next series.
Thanks.
>=20
> > + }
> > +
> > + talitos_xor_run_tx_complete_actions(desc, xor_chan);
> > +
> > + list_del(&desc->node);
> > + list_add_tail(&desc->node, &xor_chan->free_desc);
> > + spin_unlock_bh(&xor_chan->desc_lock);
> > + if (!list_empty(&xor_chan->pending_q))
> > + talitos_process_pending(xor_chan);
> > +}
> > +
> > +/**
> > + * talitos_issue_pending - move the descriptors in submit
> > + * queue to pending queue and submit them for processing
> > + * @chan: DMA channel
> > + */
> > +static void talitos_issue_pending(struct dma_chan *chan)
> > +{
> > + struct talitos_xor_chan *xor_chan;
> > +
> > + xor_chan =3D container_of(chan, struct talitos_xor_chan, common=
);
> > + spin_lock_bh(&xor_chan->desc_lock);
> > + list_splice_tail_init(&xor_chan->submit_q,
> > + &xor_chan->pending_q);
> > + spin_unlock_bh(&xor_chan->desc_lock);
> > + talitos_process_pending(xor_chan);
> > +}
> > +
> > +static dma_cookie_t talitos_async_tx_submit(struct
> dma_async_tx_descriptor *tx)
> > +{
> > + struct talitos_xor_desc *desc;
> > + struct talitos_xor_chan *xor_chan;
> > + dma_cookie_t cookie;
> > +
> > + desc =3D container_of(tx, struct talitos_xor_desc, async_tx);
> > + xor_chan =3D container_of(tx->chan, struct talitos_xor_chan,
> common);
> > +
> > + spin_lock_bh(&xor_chan->desc_lock);
> > +
> > + cookie =3D xor_chan->common.cookie + 1;
> > + if (cookie < 0)
> > + cookie =3D 1;
>=20
> Should use the new dma_cookie_assign() helper.
Ok.
>=20
> > +
> > + desc->async_tx.cookie =3D cookie;
> > + xor_chan->common.cookie =3D desc->async_tx.cookie;
> > +
> > + list_splice_tail_init(&desc->tx_list,
> > + &xor_chan->submit_q);
> > +
> > + spin_unlock_bh(&xor_chan->desc_lock);
> > +
> > + return cookie;
> > +}
> > +
> > +static struct talitos_xor_desc *talitos_xor_alloc_descriptor(
> > + struct talitos_xor_chan *xor_chan,
> gfp_t flags)
> > +{
> > + struct talitos_xor_desc *desc;
> > +
> > + desc =3D kmalloc(sizeof(*desc), flags);
> > + if (desc) {
> > + xor_chan->total_desc++;
> > + desc->async_tx.tx_submit =3D talitos_async_tx_submit;
> > + }
> > +
> > + return desc;
> > +}
> > +
> [..]
I will remove this line.
> > +
> > +static struct dma_async_tx_descriptor *talitos_prep_dma_xor(
> > + struct dma_chan *chan, dma_addr_t dest,
> dma_addr_t *src,
> > + unsigned int src_cnt, size_t len, unsigned long
> flags)
> > +{
> > + struct talitos_xor_chan *xor_chan;
> > + struct talitos_xor_desc *new;
> > + struct talitos_desc *desc;
> > + int i, j;
> > +
> > + BUG_ON(len > TALITOS_MAX_DATA_LEN);
> > +
> > + xor_chan =3D container_of(chan, struct talitos_xor_chan, common=
);
> > +
> > + spin_lock_bh(&xor_chan->desc_lock);
> > + if (!list_empty(&xor_chan->free_desc)) {
> > + new =3D container_of(xor_chan->free_desc.next,
> > + struct talitos_xor_desc, node);
> > + list_del(&new->node);
> > + } else {
> > + new =3D talitos_xor_alloc_descriptor(xor_chan,
> GFP_KERNEL | GFP_DMA);
>=20
> You can't hold a spin_lock over a GFP_KERNEL allocation.
Ok, I will fix it in next.
>=20
> > + }
> > + spin_unlock_bh(&xor_chan->desc_lock);
> > +
> > + if (!new) {
> > + dev_err(xor_chan->common.device->dev,
> > + "No free memory for XOR DMA descriptor\n");
> > + return NULL;
> > + }
> > + dma_async_tx_descriptor_init(&new->async_tx, &xor_chan->common)=
;
> > +
> > + INIT_LIST_HEAD(&new->node);
> > + INIT_LIST_HEAD(&new->tx_list);
> > +
> > + desc =3D &new->hwdesc;
> > + /* Set destination: Last pointer pair */
> > + to_talitos_ptr(&desc->ptr[6], dest);
> > + desc->ptr[6].len =3D cpu_to_be16(len);
> > + desc->ptr[6].j_extent =3D 0;
> > + new->unmap_src_cnt =3D src_cnt;
> > + new->unmap_len =3D len;
> > +
> > + /* Set Sources: End loading from second-last pointer pair */
> > + for (i =3D 5, j =3D 0; j < src_cnt && i >=3D 0; i--, j++) {
> > + to_talitos_ptr(&desc->ptr[i], src[j]);
> > + desc->ptr[i].len =3D cpu_to_be16(len);
> > + desc->ptr[i].j_extent =3D 0;
> > + }
> > +
> > + /*
> > + * documentation states first 0 ptr/len combo marks end of
> sources
> > + * yet device produces scatter boundary error unless all
> subsequent
> > + * sources are zeroed out
> > + */
> > + for (; i >=3D 0; i--) {
> > + to_talitos_ptr(&desc->ptr[i], 0);
> > + desc->ptr[i].len =3D 0;
> > + desc->ptr[i].j_extent =3D 0;
> > + }
> > +
> > + desc->hdr =3D DESC_HDR_SEL0_AESU | DESC_HDR_MODE0_AESU_XOR |
> > + DESC_HDR_TYPE_RAID_XOR;
> > +
> > + new->async_tx.parent =3D NULL;
> > + new->async_tx.next =3D NULL;
> > + new->async_tx.cookie =3D 0;
> > + async_tx_ack(&new->async_tx);
> > +
> > + list_add_tail(&new->node, &new->tx_list);
> > +
> > + new->async_tx.flags =3D flags;
> > + new->async_tx.cookie =3D -EBUSY;
> > +
> > + return &new->async_tx;
> > +}
> > +
> > +static void talitos_unregister_async_xor(struct device *dev)
> > +{
> > + struct talitos_private *priv =3D dev_get_drvdata(dev);
> > + struct talitos_xor_chan *xor_chan;
> > + struct dma_chan *chan, *_chan;
> > +
> > + if (priv->dma_dev_common.chancnt)
> > + dma_async_device_unregister(&priv->dma_dev_common);
> > +
> > + list_for_each_entry_safe(chan, _chan, &priv-
> >dma_dev_common.channels,
> > + device_node) {
> > + xor_chan =3D container_of(chan, struct talitos_xor_chan=
,
> > + common);
> > + list_del(&chan->device_node);
> > + priv->dma_dev_common.chancnt--;
> > + kfree(xor_chan);
> > + }
> > +}
> > +
> > +/**
> > + * talitos_register_dma_async - Initialize the Freescale XOR ADMA
> device
> > + * It is registered as a DMA device with the capability to perform
> > + * XOR operation with the Async_tx layer.
> > + * The various queues and channel resources are also allocated.
> > + */
> > +static int talitos_register_async_tx(struct device *dev, int
> max_xor_srcs)
> > +{
> > + struct talitos_private *priv =3D dev_get_drvdata(dev);
> > + struct dma_device *dma_dev =3D &priv->dma_dev_common;
> > + struct talitos_xor_chan *xor_chan;
> > + int err;
> > +
> > + xor_chan =3D kzalloc(sizeof(struct talitos_xor_chan), GFP_KERNE=
L);
> > + if (!xor_chan) {
> > + dev_err(dev, "unable to allocate xor channel\n");
> > + return -ENOMEM;
> > + }
> > +
> > + dma_dev->dev =3D dev;
> > + dma_dev->device_alloc_chan_resources =3D
> talitos_alloc_chan_resources;
> > + dma_dev->device_free_chan_resources =3D
> talitos_free_chan_resources;
> > + dma_dev->device_prep_dma_xor =3D talitos_prep_dma_xor;
> > + dma_dev->max_xor =3D max_xor_srcs;
> > + dma_dev->device_tx_status =3D talitos_is_tx_complete;
> > + dma_dev->device_issue_pending =3D talitos_issue_pending;
> > + INIT_LIST_HEAD(&dma_dev->channels);
> > + dma_cap_set(DMA_XOR, dma_dev->cap_mask);
> > +
> > + xor_chan->dev =3D dev;
> > + xor_chan->common.device =3D dma_dev;
> > + xor_chan->total_desc =3D 0;
> > + INIT_LIST_HEAD(&xor_chan->submit_q);
> > + INIT_LIST_HEAD(&xor_chan->pending_q);
> > + INIT_LIST_HEAD(&xor_chan->in_progress_q);
> > + INIT_LIST_HEAD(&xor_chan->free_desc);
> > + spin_lock_init(&xor_chan->desc_lock);
> > +
> > + list_add_tail(&xor_chan->common.device_node, &dma_dev-
> >channels);
> > + dma_dev->chancnt++;
> > +
> > + err =3D dma_async_device_register(dma_dev);
> > + if (err) {
> > + dev_err(dev, "Unable to register XOR with Async_tx\n");
> > + goto err_out;
> > + }
> > +
> > + return err;
> > +
> > +err_out:
> > + talitos_unregister_async_xor(dev);
> > + return err;
> > +}
> > +#endif
> > +
> > /*
> > * crypto alg
> > */
> > @@ -2891,6 +3284,26 @@ static int talitos_probe(struct platform_device
> *ofdev)
> > dev_info(dev, "hwrng\n");
> > }
> >
> > +#ifdef CONFIG_CRYPTO_DEV_TALITOS_RAIDXOR
>=20
> Kill these ifdefs in the C file. Do something like: if
> (xor_enabled()) { } around the code sections that are optional so you
> always get compile coverage. Where xor_enabled() is a shorter form of
> IS_ENABLED(CONFIG_CRYPTO_DEV_TALITOS_RAIDXOR).
I'm confused how to implement this interface,
First, this feature should be added in Kconfig (default N)
Second, the only judge condition is SEC version, but some time we don't wan=
t involve function XOR even though the SEC (version is not higher than 3.0,=
SEC 4.0 is CAAM module) support this feature.
Last, there is not any hardware support.
Can you give me an example? Thanks.
^ permalink raw reply
* RE: [PATCH v7 6/8] fsl-dma: use spin_lock_bh to instead of spin_lock_irqsave
From: Liu Qiang-B32616 @ 2012-09-04 12:39 UTC (permalink / raw)
To: Dan Williams
Cc: herbert@gondor.apana.org.au, arnd@arndb.de, vinod.koul@intel.com,
gregkh@linuxfoundation.org, Tabi Timur-B04825,
Phillips Kim-R1AAHA, linux-kernel@vger.kernel.org,
linux-crypto@vger.kernel.org, linuxppc-dev@lists.ozlabs.org,
davem@davemloft.net, Li Yang-R58472
In-Reply-To: <CAA9_cmeFamCX1u+z3fP_gxAqe83JFbY9Beqpqgb-PjRoJ8BrWA@mail.gmail.com>
> -----Original Message-----
> From: dan.j.williams@gmail.com [mailto:dan.j.williams@gmail.com] On
> Behalf Of Dan Williams
> Sent: Sunday, September 02, 2012 4:41 PM
> To: Liu Qiang-B32616
> Cc: linux-crypto@vger.kernel.org; linuxppc-dev@lists.ozlabs.org; linux-
> kernel@vger.kernel.org; vinod.koul@intel.com; Phillips Kim-R1AAHA;
> herbert@gondor.hengli.com.au; davem@davemloft.net; arnd@arndb.de;
> gregkh@linuxfoundation.org; Li Yang-R58472; Tabi Timur-B04825
> Subject: Re: [PATCH v7 6/8] fsl-dma: use spin_lock_bh to instead of
> spin_lock_irqsave
>=20
> On Thu, Aug 9, 2012 at 1:23 AM, <qiang.liu@freescale.com> wrote:
> > From: Qiang Liu <qiang.liu@freescale.com>
> >
> > The use of spin_lock_irqsave() is a stronger locking mechanism than is
> > required throughout the driver. The minimum locking required should be
> > used instead. Interrupts will be turned off and context will be saved,
> > there is needless to use irqsave.
> >
> > Change all instances of spin_lock_irqsave() to spin_lock_bh().
> > All manipulation of protected fields is done using tasklet context or
> > weaker, which makes spin_lock_bh() the correct choice.
>=20
> It seems you are coordinating fsl-dma copy and talitos xor operations.
> It looks like fsl-dma will be called through
> talitos_process_pending()->dma_run_dependencies(), which is
> potentially called in hard irq context.
>=20
> This all comes back to the need to fix raid offload to manage the
> channels explicitly rather than the current dependency chains.
So you mean I must implement talitos_run_dependencies() and fsldma_run_depe=
ndencies()? Invoke async_tx->callback() respectively.
How about avoiding irq context in talitos?
>=20
> --
> Dan
^ permalink raw reply
* Re: 3.5+: yaboot, Invalid memory access
From: Steven Rostedt @ 2012-09-04 14:27 UTC (permalink / raw)
To: Michael Ellerman; +Cc: Christian Kujau, linuxppc-dev
In-Reply-To: <1346741491.7619.12.camel@concordia>
On Tue, 2012-09-04 at 16:51 +1000, Michael Ellerman wrote:
> My guess would be we're calling that quite early and the __put_user()
> check is getting confused and failing. That means we'll have left some
> code unpatched, which then fails.
>
> Can you try with the patch applied, but instead of returning if the
> __put_user() fails, just continue on anyway.
>
> That will isolate if it's something in the __put_user() (I doubt it), or
> just that the __put_user() is failing and leaving the code unpatched.
As a test case, continuing on error is fine, but I would not recommend
that as a fix. If it fails, but still does the patch, that could be
harmful, and confusing of a result.
Need to figure out why put_user is failing.
-- Steve
^ permalink raw reply
* RE: [PATCH] sata_fsl: add workaround for data length mismatch on freescale V2 controller
From: David Laight @ 2012-09-04 14:51 UTC (permalink / raw)
To: Shaohui Xie, jgarzik, linux-ide; +Cc: linuxppc-dev, linux-kernel, Anju Bhartiya
In-Reply-To: <1346756920-19128-1-git-send-email-Shaohui.Xie@freescale.com>
> + /* Read command completed register */
> + done_mask =3D ioread32(hcr_base + CC);
> +
> + if (host_priv->quirks & SATA_FSL_QUIRK_V2_ERRATA) {
> + if (unlikely(hstatus & INT_ON_DATA_LENGTH_MISMATCH)) {
> + for (tag =3D 0; tag < ATA_MAX_QUEUE; tag++) {
> + qc =3D ata_qc_from_tag(ap, tag);
> + if (qc && ata_is_atapi(qc->tf.protocol))
{
> + atapi_flag =3D 1;
> + break;
> + }
> + }
> + }
> + }
> +
> + /* Workaround for data length mismatch errata */
> + if (atapi_flag) {
Seems to me like the conditionals for this code are all
in the wrong order - adding code to the normal path.
The whole lot should probably be inside:
if (unlikely(hstatus & INT_ON_DATA_LENGTH_MISMATCH)) {
and the 'atapi_flag' boolean removed.
I also wonder it this is worthy of an actual quirk?
Might be worth doing anyway.
David
^ permalink raw reply
* [PATCH] usb: gadget: fsl_udc_core: remove mapped flag
From: Enrico Scholz @ 2012-09-04 17:24 UTC (permalink / raw)
To: linux-usb, linuxppc-dev; +Cc: gregkh, balbi, Enrico Scholz
The 'mapped' flag in 'struct fsl_req' flag is redundant with checking
for 'req.dma != DMA_ADDR_INVALID' and it was also set to a wrong value
(see 2nd hunk of patch).
Replacing it in the way described above saves 60 bytes:
function old new delta
fsl_udc_irq 2952 2940 -12
ep0_prime_status 380 368 -12
done 448 432 -16
fsl_ep_queue 668 648 -20
and has same (or less) runtime costs like evaluating 'req->mapped'.
Signed-off-by: Enrico Scholz <enrico.scholz@sigma-chemnitz.de>
---
drivers/usb/gadget/fsl_udc_core.c | 10 ++--------
drivers/usb/gadget/fsl_usb2_udc.h | 1 -
2 files changed, 2 insertions(+), 9 deletions(-)
diff --git a/drivers/usb/gadget/fsl_udc_core.c b/drivers/usb/gadget/fsl_udc_core.c
index 55c4a61..1282a11 100644
--- a/drivers/usb/gadget/fsl_udc_core.c
+++ b/drivers/usb/gadget/fsl_udc_core.c
@@ -195,14 +195,13 @@ static void done(struct fsl_ep *ep, struct fsl_req *req, int status)
dma_pool_free(udc->td_pool, curr_td, curr_td->td_dma);
}
- if (req->mapped) {
+ if (req->req.dma != DMA_ADDR_INVALID) {
dma_unmap_single(ep->udc->gadget.dev.parent,
req->req.dma, req->req.length,
ep_is_in(ep)
? DMA_TO_DEVICE
: DMA_FROM_DEVICE);
req->req.dma = DMA_ADDR_INVALID;
- req->mapped = 0;
} else
dma_sync_single_for_cpu(ep->udc->gadget.dev.parent,
req->req.dma, req->req.length,
@@ -915,15 +914,12 @@ fsl_ep_queue(struct usb_ep *_ep, struct usb_request *_req, gfp_t gfp_flags)
req->req.length, ep_is_in(ep)
? DMA_TO_DEVICE
: DMA_FROM_DEVICE);
- req->mapped = 1;
- } else {
+ } else
dma_sync_single_for_device(ep->udc->gadget.dev.parent,
req->req.dma, req->req.length,
ep_is_in(ep)
? DMA_TO_DEVICE
: DMA_FROM_DEVICE);
- req->mapped = 0;
- }
req->req.status = -EINPROGRESS;
req->req.actual = 0;
@@ -1306,7 +1302,6 @@ static int ep0_prime_status(struct fsl_udc *udc, int direction)
req->req.dma = dma_map_single(ep->udc->gadget.dev.parent,
req->req.buf, req->req.length,
ep_is_in(ep) ? DMA_TO_DEVICE : DMA_FROM_DEVICE);
- req->mapped = 1;
if (fsl_req_to_dtd(req, GFP_ATOMIC) == 0)
fsl_queue_td(ep, req);
@@ -1389,7 +1384,6 @@ static void ch9getstatus(struct fsl_udc *udc, u8 request_type, u16 value,
req->req.dma = dma_map_single(ep->udc->gadget.dev.parent,
req->req.buf, req->req.length,
ep_is_in(ep) ? DMA_TO_DEVICE : DMA_FROM_DEVICE);
- req->mapped = 1;
/* prime the data phase */
if ((fsl_req_to_dtd(req, GFP_ATOMIC) == 0))
diff --git a/drivers/usb/gadget/fsl_usb2_udc.h b/drivers/usb/gadget/fsl_usb2_udc.h
index fbd77ba..9aab166 100644
--- a/drivers/usb/gadget/fsl_usb2_udc.h
+++ b/drivers/usb/gadget/fsl_usb2_udc.h
@@ -436,7 +436,6 @@ struct fsl_req {
/* ep_queue() func will add
a request->queue into a udc_ep->queue 'd tail */
struct fsl_ep *ep;
- unsigned mapped:1;
struct ep_td_struct *head, *tail; /* For dTD List
cpu endian Virtual addr */
--
1.7.11.4
^ permalink raw reply related
* [PATCH] usb: gadget: fsl_udc_core: do not immediatly prime STATUS for IN xfer
From: Enrico Scholz @ 2012-09-04 16:58 UTC (permalink / raw)
To: linux-usb, linuxppc-dev; +Cc: gregkh, balbi, Enrico Scholz
Because the fsl_udc_core driver shares one 'status_req' object for the
complete ep0 control transfer, it is not possible to prime the final
STATUS phase immediately after the IN transaction. E.g. ch9getstatus()
executed:
| req = udc->status_req;
| ...
| list_add_tail(&req->queue, &ep->queue);
| if (ep0_prime_status(udc, EP_DIR_OUT))
| ....
| struct fsl_req *req = udc->status_req;
| list_add_tail(&req->queue, &ep->queue);
which corrupts the ep->queue list by inserting 'status_req' twice. This
causes a kernel oops e.g. when 'lsusb -v' is executed on the host.
Patch delays the final 'ep0_prime_status(udc, EP_DIR_OUT))' by moving it
into the ep0 completion handler.
Signed-off-by: Enrico Scholz <enrico.scholz@sigma-chemnitz.de>
---
drivers/usb/gadget/fsl_udc_core.c | 16 +++-------------
1 file changed, 3 insertions(+), 13 deletions(-)
diff --git a/drivers/usb/gadget/fsl_udc_core.c b/drivers/usb/gadget/fsl_udc_core.c
index d7138cc..55c4a61 100644
--- a/drivers/usb/gadget/fsl_udc_core.c
+++ b/drivers/usb/gadget/fsl_udc_core.c
@@ -1294,8 +1294,7 @@ static int ep0_prime_status(struct fsl_udc *udc, int direction)
udc->ep0_dir = USB_DIR_OUT;
ep = &udc->eps[0];
- if (udc->ep0_state != DATA_STATE_XMIT)
- udc->ep0_state = WAIT_FOR_OUT_STATUS;
+ udc->ep0_state = WAIT_FOR_OUT_STATUS;
req->ep = ep;
req->req.length = 0;
@@ -1400,8 +1399,6 @@ static void ch9getstatus(struct fsl_udc *udc, u8 request_type, u16 value,
list_add_tail(&req->queue, &ep->queue);
udc->ep0_state = DATA_STATE_XMIT;
- if (ep0_prime_status(udc, EP_DIR_OUT))
- ep0stall(udc);
return;
stall:
@@ -1511,14 +1508,6 @@ static void setup_received_irq(struct fsl_udc *udc,
spin_lock(&udc->lock);
udc->ep0_state = (setup->bRequestType & USB_DIR_IN)
? DATA_STATE_XMIT : DATA_STATE_RECV;
- /*
- * If the data stage is IN, send status prime immediately.
- * See 2.0 Spec chapter 8.5.3.3 for detail.
- */
- if (udc->ep0_state == DATA_STATE_XMIT)
- if (ep0_prime_status(udc, EP_DIR_OUT))
- ep0stall(udc);
-
} else {
/* No data phase, IN status from gadget */
udc->ep0_dir = USB_DIR_IN;
@@ -1548,7 +1537,8 @@ static void ep0_req_complete(struct fsl_udc *udc, struct fsl_ep *ep0,
switch (udc->ep0_state) {
case DATA_STATE_XMIT:
/* already primed at setup_received_irq */
- udc->ep0_state = WAIT_FOR_OUT_STATUS;
+ if (ep0_prime_status(udc, EP_DIR_OUT))
+ ep0stall(udc);
break;
case DATA_STATE_RECV:
/* send status phase */
--
1.7.11.4
^ permalink raw reply related
* Re: 3.5+: yaboot, Invalid memory access
From: Benjamin Herrenschmidt @ 2012-09-04 19:40 UTC (permalink / raw)
To: Michael Ellerman; +Cc: Christian Kujau, linuxppc-dev, Steven Rostedt
In-Reply-To: <1346741491.7619.12.camel@concordia>
On Tue, 2012-09-04 at 16:51 +1000, Michael Ellerman wrote:
>
> My guess would be we're calling that quite early and the __put_user()
> check is getting confused and failing. That means we'll have left some
> code unpatched, which then fails.
>
> Can you try with the patch applied, but instead of returning if the
> __put_user() fails, just continue on anyway.
>
> That will isolate if it's something in the __put_user() (I doubt it),
> or
> just that the __put_user() is failing and leaving the code unpatched.
Bah, worse.... we do the feature fixups on 32-bit while still running
unrelocated, so we need RELOC() macros everywhere. That's probably
busted.
We might be able to try replacing __put_user() with __put_user_size() to
avoid the kernel address check, which is I think what's breaking it.
That stuff has long been in my list of things to rework, ie, we do that
fixup way too early on ppc32, in fact earlier than ppc64, ie, before the
platform is probed which means the platform cannot adjust the bits.
However, iirc, we have some early setup code that relies on the fixup
being done that early so it's a bit of a chicken & egg problem that
needs to be untangled first.
Cheers,
Ben.
^ permalink raw reply
* Re: [PATCH] powerpc-powernv: added tce_get callback for powernv platform
From: Benjamin Herrenschmidt @ 2012-09-04 19:41 UTC (permalink / raw)
To: Alexey Kardashevskiy; +Cc: linuxppc-dev, Paul Mackerras, David Gibson
In-Reply-To: <1346744158-31190-1-git-send-email-aik@ozlabs.ru>
On Tue, 2012-09-04 at 17:35 +1000, Alexey Kardashevskiy wrote:
> The upcoming VFIO support requires a way to know which
> entry in the TCE map is not empty in order to do cleanup
> at QEMU exit/crash. This patch adds such functionality
> to POWERNV platform code.
>
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> ---
> arch/powerpc/platforms/powernv/pci.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/arch/powerpc/platforms/powernv/pci.c b/arch/powerpc/platforms/powernv/pci.c
> index be3cfc5..61f8068 100644
> --- a/arch/powerpc/platforms/powernv/pci.c
> +++ b/arch/powerpc/platforms/powernv/pci.c
> @@ -447,6 +447,11 @@ static void pnv_tce_free(struct iommu_table *tbl, long index, long npages)
> pnv_tce_invalidate(tbl, tces, tcep - 1);
> }
>
> +static unsigned long pnv_tce_get(struct iommu_table *tbl, long index)
> +{
> + return ((u64 *)tbl->it_base)[index - tbl->it_offset] & IOMMU_PAGE_MASK;
> +}
Why the masking here ?
Cheers,
Ben.
> void pnv_pci_setup_iommu_table(struct iommu_table *tbl,
> void *tce_mem, u64 tce_size,
> u64 dma_offset)
> @@ -597,6 +602,7 @@ void __init pnv_pci_init(void)
> ppc_md.pci_dma_dev_setup = pnv_pci_dma_dev_setup;
> ppc_md.tce_build = pnv_tce_build;
> ppc_md.tce_free = pnv_tce_free;
> + ppc_md.tce_get = pnv_tce_get;
> ppc_md.pci_probe_mode = pnv_pci_probe_mode;
> set_pci_dma_ops(&dma_iommu_ops);
>
^ permalink raw reply
* Re: [PATCH] powerpc-powernv: align BARs to PAGE_SIZE on powernv platform
From: Benjamin Herrenschmidt @ 2012-09-04 19:45 UTC (permalink / raw)
To: Alexey Kardashevskiy
Cc: linuxppc-dev, Alex Williamson, Paul Mackerras, David Gibson
In-Reply-To: <1346744201-31262-1-git-send-email-aik@ozlabs.ru>
On Tue, 2012-09-04 at 17:36 +1000, Alexey Kardashevskiy wrote:
> VFIO adds a separate memory region for every BAR and tries
> to mmap() it to provide direct BAR mapping to the guest.
> If it succeedes, QEMU registers this address with kvm_set_phys_mem().
> However it is not always possible because such a BAR should
> be host page size aligned. In this case VFIO uses "slow" path
> and emulated BAR access in QEMU.
>
> In order to avoid "slow" path, BARs have to be PAGE_SIZE aligned
> in the host kernel and this is what the patch does.
>
> The patch adds powernv platform specific hook which makes all
> BARs sizes 64K aligned. The pci_reassigndev_resource_alignment()
> function from drivers/pci/pci.c has been used as a reference.
>
> This is purely an optimization patch, the things will work without
> it, just a bit slower.
It's still bad in more ways that I care to explain...
The main one is that you do the "fixup" in a very wrong place anyway and
it might cause cases of overlapping BARs.
In any case this is wrong. It's a VFIO design bug and needs to be fixed
there (CC'ing Alex).
IE. We need a way to know where the BAR is within a page at which point
VFIO can still map the page, but can also properly take into account the
offset.
We also need a way to tell VFIO userspace that it's OK to use the fast
path for such small BARs. It's not for all host platforms. We know it's
ok for PowerNV because we know the devices are grouped by PEs and the PE
granularity is larger than a page but that's not necessarily going to be
the case on all powerpc platforms that support KVM.
Cheers,
Ben.
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> ---
> arch/powerpc/platforms/powernv/setup.c | 26 ++++++++++++++++++++++++++
> 1 file changed, 26 insertions(+)
>
> diff --git a/arch/powerpc/platforms/powernv/setup.c b/arch/powerpc/platforms/powernv/setup.c
> index db1ad1c..331838e 100644
> --- a/arch/powerpc/platforms/powernv/setup.c
> +++ b/arch/powerpc/platforms/powernv/setup.c
> @@ -25,6 +25,7 @@
> #include <linux/of.h>
> #include <linux/interrupt.h>
> #include <linux/bug.h>
> +#include <linux/pci.h>
>
> #include <asm/machdep.h>
> #include <asm/firmware.h>
> @@ -179,6 +180,30 @@ static int __init pnv_probe(void)
> return 1;
> }
>
> +static void pnv_pcibios_fixup_resources(struct pci_dev *pdev)
> +{
> + struct resource *r;
> + int i;
> +
> + /*
> + * Aligning resources to PAGE_SIZE in order to
> + * support "fast" path for PCI BAR access under VFIO
> + * which maps every BAR individually to the guest
> + * so BARs have to be PAGE aligned.
> + */
> + for (i = 0; i <= PCI_ROM_RESOURCE; i++) {
> + r = &pdev->resource[i];
> + if (!r->flags)
> + continue;
> + pr_debug("powernv: %s, aligning BAR#%d %llx..%llx",
> + pdev->dev.kobj.name, i, r->start, r->end);
> + r->end = PAGE_ALIGN(r->end - r->start + 1) - 1;
> + r->start = 0;
> + r->flags |= IORESOURCE_UNSET;
> + pr_debug(" to %llx..%llx\n", r->start, r->end);
> + }
> +}
> +
> define_machine(powernv) {
> .name = "PowerNV",
> .probe = pnv_probe,
> @@ -189,6 +214,7 @@ define_machine(powernv) {
> .progress = pnv_progress,
> .power_save = power7_idle,
> .calibrate_decr = generic_calibrate_decr,
> + .pcibios_fixup_resources= pnv_pcibios_fixup_resources,
> #ifdef CONFIG_KEXEC
> .kexec_cpu_down = pnv_kexec_cpu_down,
> #endif
^ permalink raw reply
* Re: 3.5+: yaboot, Invalid memory access
From: Benjamin Herrenschmidt @ 2012-09-04 19:49 UTC (permalink / raw)
To: Steven Rostedt; +Cc: Christian Kujau, linuxppc-dev
In-Reply-To: <1346768840.27919.2.camel@gandalf.local.home>
On Tue, 2012-09-04 at 10:27 -0400, Steven Rostedt wrote:
> As a test case, continuing on error is fine, but I would not recommend
> that as a fix. If it fails, but still does the patch, that could be
> harmful, and confusing of a result.
>
> Need to figure out why put_user is failing.
It's probably not failiing... it's just called in a context in very
early boot on ppc32 where the might_sleep() test in there will trip
(because we are running before we are operating at our link address) and
will crash horribly.
I'll try to figure out a good way to fix this today.
Cheers,
Ben.
^ permalink raw reply
* Re: [PATCH] powerpc-powernv: added tce_get callback for powernv platform
From: David Gibson @ 2012-09-04 22:35 UTC (permalink / raw)
To: Benjamin Herrenschmidt; +Cc: Alexey Kardashevskiy, Paul Mackerras, linuxppc-dev
In-Reply-To: <1346787702.3025.7.camel@pasglop>
On Wed, Sep 05, 2012 at 05:41:42AM +1000, Benjamin Herrenschmidt wrote:
> On Tue, 2012-09-04 at 17:35 +1000, Alexey Kardashevskiy wrote:
> > The upcoming VFIO support requires a way to know which
> > entry in the TCE map is not empty in order to do cleanup
> > at QEMU exit/crash. This patch adds such functionality
> > to POWERNV platform code.
> >
> > Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> > ---
> > arch/powerpc/platforms/powernv/pci.c | 6 ++++++
> > 1 file changed, 6 insertions(+)
> >
> > diff --git a/arch/powerpc/platforms/powernv/pci.c b/arch/powerpc/platforms/powernv/pci.c
> > index be3cfc5..61f8068 100644
> > --- a/arch/powerpc/platforms/powernv/pci.c
> > +++ b/arch/powerpc/platforms/powernv/pci.c
> > @@ -447,6 +447,11 @@ static void pnv_tce_free(struct iommu_table *tbl, long index, long npages)
> > pnv_tce_invalidate(tbl, tces, tcep - 1);
> > }
> >
> > +static unsigned long pnv_tce_get(struct iommu_table *tbl, long index)
> > +{
> > + return ((u64 *)tbl->it_base)[index - tbl->it_offset] & IOMMU_PAGE_MASK;
> > +}
>
> Why the masking here ?
Yes. Especially since you're masking out the permission bits which
are actually the ones you want to determine if a TCE is empty or not.
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
^ permalink raw reply
* Re: [RFC v8 PATCH 08/20] memory-hotplug: remove /sys/firmware/memmap/X sysfs
From: Andrew Morton @ 2012-09-04 23:16 UTC (permalink / raw)
To: Wen Congyang
Cc: linux-s390, linux-ia64, len.brown, linux-acpi, linux-sh, x86,
linux-kernel, cmetcalf, linux-mm, isimatu.yasuaki, paulus,
minchan.kim, kosaki.motohiro, rientjes, sparclinux,
Christoph Lameter, cl, linuxppc-dev, liuj97
In-Reply-To: <5044454E.7070909@cn.fujitsu.com>
On Mon, 03 Sep 2012 13:51:10 +0800
Wen Congyang <wency@cn.fujitsu.com> wrote:
> >> +static void release_firmware_map_entry(struct kobject *kobj)
> >> +{
> >> + struct firmware_map_entry *entry = to_memmap_entry(kobj);
> >> + struct page *page;
> >> +
> >> + page = virt_to_page(entry);
> >> + if (PageSlab(page) || PageCompound(page))
> >
> > That PageCompound() test looks rather odd. Why is this done?
>
> Liu Jiang and Christoph Lameter discussed how to find slab page
> in this mail:
> https://lkml.org/lkml/2012/7/6/333.
Well, please add a code comment to release_firmware_map_entry() which
fully explains these things.
I see that Christoph and I agree: "It would be cleaner if memory
hotplug had an indicator which allocation mechanism was used and would
use the corresponding free action". You didn't respond to this
suggestion when he made it, nor when I made it. What are your thoughts
on this?
^ permalink raw reply
* Re: [PATCH] powerpc-powernv: added tce_get callback for powernv platform
From: Alexey Kardashevskiy @ 2012-09-05 0:19 UTC (permalink / raw)
To: Benjamin Herrenschmidt; +Cc: linuxppc-dev, Paul Mackerras, David Gibson
In-Reply-To: <1346787702.3025.7.camel@pasglop>
On 05/09/12 05:41, Benjamin Herrenschmidt wrote:
> On Tue, 2012-09-04 at 17:35 +1000, Alexey Kardashevskiy wrote:
>> The upcoming VFIO support requires a way to know which
>> entry in the TCE map is not empty in order to do cleanup
>> at QEMU exit/crash. This patch adds such functionality
>> to POWERNV platform code.
>>
>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>> ---
>> arch/powerpc/platforms/powernv/pci.c | 6 ++++++
>> 1 file changed, 6 insertions(+)
>>
>> diff --git a/arch/powerpc/platforms/powernv/pci.c b/arch/powerpc/platforms/powernv/pci.c
>> index be3cfc5..61f8068 100644
>> --- a/arch/powerpc/platforms/powernv/pci.c
>> +++ b/arch/powerpc/platforms/powernv/pci.c
>> @@ -447,6 +447,11 @@ static void pnv_tce_free(struct iommu_table *tbl, long index, long npages)
>> pnv_tce_invalidate(tbl, tces, tcep - 1);
>> }
>>
>> +static unsigned long pnv_tce_get(struct iommu_table *tbl, long index)
>> +{
>> + return ((u64 *)tbl->it_base)[index - tbl->it_offset] & IOMMU_PAGE_MASK;
>> +}
>
> Why the masking here ?
Oops. No reason. Will remove.
>
> Cheers,
> Ben.
>
>> void pnv_pci_setup_iommu_table(struct iommu_table *tbl,
>> void *tce_mem, u64 tce_size,
>> u64 dma_offset)
>> @@ -597,6 +602,7 @@ void __init pnv_pci_init(void)
>> ppc_md.pci_dma_dev_setup = pnv_pci_dma_dev_setup;
>> ppc_md.tce_build = pnv_tce_build;
>> ppc_md.tce_free = pnv_tce_free;
>> + ppc_md.tce_get = pnv_tce_get;
>> ppc_md.pci_probe_mode = pnv_pci_probe_mode;
>> set_pci_dma_ops(&dma_iommu_ops);
>>
>
>
--
Alexey
^ permalink raw reply
* Re: [PATCH] powerpc-powernv: added tce_get callback for powernv platform
From: Benjamin Herrenschmidt @ 2012-09-05 0:32 UTC (permalink / raw)
To: Alexey Kardashevskiy; +Cc: linuxppc-dev, Paul Mackerras, David Gibson
In-Reply-To: <50469A95.7050606@ozlabs.ru>
On Wed, 2012-09-05 at 10:19 +1000, Alexey Kardashevskiy wrote:
> >> +static unsigned long pnv_tce_get(struct iommu_table *tbl, long
> index)
> >> +{
> >> + return ((u64 *)tbl->it_base)[index - tbl->it_offset] &
> IOMMU_PAGE_MASK;
> >> +}
> >
> > Why the masking here ?
>
>
> Oops. No reason. Will remove.
Right. The caller wants to know both whether the low bits are set and
whether there's an address up.
On the H_PUT_TCE path, you want to make sure:
- If any of the low bit is set, set the TCE entry & get_page()
- If none, then clear the whole entry (ignore the high bits passed by
the guest) and maybe put_page() the old page
IE the TCE either contains a valid page address + low bit(s) or all 0
That way, on the cleanup path, you can check the low bits only to decide
whether to cleanup, and if any is set, you know both your direction
(writeable vs. read only) and whether something was there at all.
You do not want to ever compare the high bits (address) to 0. While we
never do it in practice I suspect, there's no fundamental reason why a
physical address of 0 is incorrect in a TCE.
Cheers,
Ben.
^ permalink raw reply
* Re: [PATCH] powerpc-powernv: align BARs to PAGE_SIZE on powernv platform
From: Alexey Kardashevskiy @ 2012-09-05 0:55 UTC (permalink / raw)
To: Benjamin Herrenschmidt
Cc: linuxppc-dev, Alex Williamson, Paul Mackerras, David Gibson
In-Reply-To: <1346787940.3025.11.camel@pasglop>
On 05/09/12 05:45, Benjamin Herrenschmidt wrote:
> On Tue, 2012-09-04 at 17:36 +1000, Alexey Kardashevskiy wrote:
>> VFIO adds a separate memory region for every BAR and tries
>> to mmap() it to provide direct BAR mapping to the guest.
>> If it succeedes, QEMU registers this address with kvm_set_phys_mem().
>> However it is not always possible because such a BAR should
>> be host page size aligned. In this case VFIO uses "slow" path
>> and emulated BAR access in QEMU.
>>
>> In order to avoid "slow" path, BARs have to be PAGE_SIZE aligned
>> in the host kernel and this is what the patch does.
>>
>> The patch adds powernv platform specific hook which makes all
>> BARs sizes 64K aligned. The pci_reassigndev_resource_alignment()
>> function from drivers/pci/pci.c has been used as a reference.
>>
>> This is purely an optimization patch, the things will work without
>> it, just a bit slower.
>
> It's still bad in more ways that I care to explain...
Well it is right before pci_reassigndev_resource_alignment() which is
common and does the same thing.
> The main one is that you do the "fixup" in a very wrong place anyway and
> it might cause cases of overlapping BARs.
As far as I can tell it may only happen if someone tries to align resource
via kernel command line.
But ok. I trust you :)
> In any case this is wrong. It's a VFIO design bug and needs to be fixed
> there (CC'ing Alex).
It can be fixed in VFIO only if VFIO will stop treating functions
separately and start mapping group's MMIO space as a whole thing. But this
is not going to happen.
The example of the problem is NEC USB PCI which has 3 functions, each has
one BAR, these BARs are 4K aligned and I cannot see how it can be fixed
with 64K page size and VFIO creating memory regions per BAR (not per PHB).
> IE. We need a way to know where the BAR is within a page at which point
> VFIO can still map the page, but can also properly take into account the
> offset.
It is not about VFIO, it is about KVM. I cannot put non-aligned page to
kvm_set_phys_mem(). Cannot understand how we would solve this.
You better discuss it with David, my vocab is weak.
> We also need a way to tell VFIO userspace that it's OK to use the fast
> path for such small BARs. It's not for all host platforms. We know it's
> ok for PowerNV because we know the devices are grouped by PEs and the PE
> granularity is larger than a page but that's not necessarily going to be
> the case on all powerpc platforms that support KVM.
>
> Cheers,
> Ben.
>
>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>> ---
>> arch/powerpc/platforms/powernv/setup.c | 26 ++++++++++++++++++++++++++
>> 1 file changed, 26 insertions(+)
>>
>> diff --git a/arch/powerpc/platforms/powernv/setup.c b/arch/powerpc/platforms/powernv/setup.c
>> index db1ad1c..331838e 100644
>> --- a/arch/powerpc/platforms/powernv/setup.c
>> +++ b/arch/powerpc/platforms/powernv/setup.c
>> @@ -25,6 +25,7 @@
>> #include <linux/of.h>
>> #include <linux/interrupt.h>
>> #include <linux/bug.h>
>> +#include <linux/pci.h>
>>
>> #include <asm/machdep.h>
>> #include <asm/firmware.h>
>> @@ -179,6 +180,30 @@ static int __init pnv_probe(void)
>> return 1;
>> }
>>
>> +static void pnv_pcibios_fixup_resources(struct pci_dev *pdev)
>> +{
>> + struct resource *r;
>> + int i;
>> +
>> + /*
>> + * Aligning resources to PAGE_SIZE in order to
>> + * support "fast" path for PCI BAR access under VFIO
>> + * which maps every BAR individually to the guest
>> + * so BARs have to be PAGE aligned.
>> + */
>> + for (i = 0; i <= PCI_ROM_RESOURCE; i++) {
>> + r = &pdev->resource[i];
>> + if (!r->flags)
>> + continue;
>> + pr_debug("powernv: %s, aligning BAR#%d %llx..%llx",
>> + pdev->dev.kobj.name, i, r->start, r->end);
>> + r->end = PAGE_ALIGN(r->end - r->start + 1) - 1;
>> + r->start = 0;
>> + r->flags |= IORESOURCE_UNSET;
>> + pr_debug(" to %llx..%llx\n", r->start, r->end);
>> + }
>> +}
>> +
>> define_machine(powernv) {
>> .name = "PowerNV",
>> .probe = pnv_probe,
>> @@ -189,6 +214,7 @@ define_machine(powernv) {
>> .progress = pnv_progress,
>> .power_save = power7_idle,
>> .calibrate_decr = generic_calibrate_decr,
>> + .pcibios_fixup_resources= pnv_pcibios_fixup_resources,
>> #ifdef CONFIG_KEXEC
>> .kexec_cpu_down = pnv_kexec_cpu_down,
>> #endif
>
>
--
Alexey
^ permalink raw reply
* Re: 3.5+: yaboot, Invalid memory access
From: Benjamin Herrenschmidt @ 2012-09-05 1:08 UTC (permalink / raw)
To: Christian Kujau; +Cc: linuxppc-dev, Steven Rostedt
In-Reply-To: <alpine.DEB.2.01.1209040223380.25392@trent.utfs.org>
On Tue, 2012-09-04 at 02:32 -0700, Christian Kujau wrote:
> On Tue, 4 Sep 2012 at 16:51, Michael Ellerman wrote:
> > My guess would be we're calling that quite early and the __put_user()
> > check is getting confused and failing. That means we'll have left some
> > code unpatched, which then fails.
> >
> > Can you try with the patch applied, but instead of returning if the
> > __put_user() fails, just continue on anyway.
>
> You mean, like this?
Try this:
powerpc: Don't use __put_user() in patch_instruction
patch_instruction() can be called very early on ppc32, when the kernel
isn't yet running at it's linked address. That can cause the !
is_kernel_addr() test in __put_user() to trip and call might_sleep()
which is very bad at that point during boot.
Use a lower level function instead for now, at least until we get to
rework ppc32 boot process to do the code patching later, like ppc64
does.
Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
diff --git a/arch/powerpc/lib/code-patching.c b/arch/powerpc/lib/code-patching.c
index dd223b3..17e5b23 100644
--- a/arch/powerpc/lib/code-patching.c
+++ b/arch/powerpc/lib/code-patching.c
@@ -20,7 +20,7 @@ int patch_instruction(unsigned int *addr, unsigned int instr)
{
int err;
- err = __put_user(instr, addr);
+ __put_user_size(instr, addr, 4, err);
if (err)
return err;
asm ("dcbst 0, %0; sync; icbi 0,%0; sync; isync" : : "r" (addr));
^ permalink raw reply related
* Re: [PATCH] powerpc-powernv: align BARs to PAGE_SIZE on powernv platform
From: Benjamin Herrenschmidt @ 2012-09-05 1:16 UTC (permalink / raw)
To: Alexey Kardashevskiy
Cc: linuxppc-dev, Alex Williamson, Paul Mackerras, David Gibson
In-Reply-To: <5046A2F1.1020004@ozlabs.ru>
> > It's still bad in more ways that I care to explain...
>
> Well it is right before pci_reassigndev_resource_alignment() which is
> common and does the same thing.
>
> > The main one is that you do the "fixup" in a very wrong place anyway and
> > it might cause cases of overlapping BARs.
>
> As far as I can tell it may only happen if someone tries to align resource
> via kernel command line.
>
> But ok. I trust you :)
I have reasons to believe that this realignment crap is wrong too :-)
> > In any case this is wrong. It's a VFIO design bug and needs to be fixed
> > there (CC'ing Alex).
>
> It can be fixed in VFIO only if VFIO will stop treating functions
> separately and start mapping group's MMIO space as a whole thing. But this
> is not going to happen.
It still can be fixed without that...
> The example of the problem is NEC USB PCI which has 3 functions, each has
> one BAR, these BARs are 4K aligned and I cannot see how it can be fixed
> with 64K page size and VFIO creating memory regions per BAR (not per PHB).
VFIO can perfectly well realize it's the same MR or even map the same
area 3 times and create 3 MRs, both options work. All it needs is to
know the offset of the BAR inside the page.
> > IE. We need a way to know where the BAR is within a page at which point
> > VFIO can still map the page, but can also properly take into account the
> > offset.
>
> It is not about VFIO, it is about KVM. I cannot put non-aligned page to
> kvm_set_phys_mem(). Cannot understand how we would solve this.
No, VFIO still maps the whole page and creates an MR for the whole page,
that's fine. But you still need to know the offset within the page.
Now the main problem here is going to be that the guest itself might
reallocate the BAR and move it around (well, it's version of the BAR
which isn't the real thing), and so we cannot create a direct MMU
mapping between -that- and the real BAR.
IE. We can only allow that direct mapping if the guest BAR mapping has
the same "offset within page" as the host BAR mapping.
Our guests don't mess with BARs but SLOF does ... it's really tempting
to look into bringing the whole BAR allocation back into qemu and out of
SLOF :-( (We might have to if we ever do hotplug anyway). That way qemu
could set offsets that match appropriately.
Cheers,
Ben.
> You better discuss it with David, my vocab is weak.
>
>
>
> > We also need a way to tell VFIO userspace that it's OK to use the fast
> > path for such small BARs. It's not for all host platforms. We know it's
> > ok for PowerNV because we know the devices are grouped by PEs and the PE
> > granularity is larger than a page but that's not necessarily going to be
> > the case on all powerpc platforms that support KVM.
> >
> > Cheers,
> > Ben.
> >
> >> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> >> ---
> >> arch/powerpc/platforms/powernv/setup.c | 26 ++++++++++++++++++++++++++
> >> 1 file changed, 26 insertions(+)
> >>
> >> diff --git a/arch/powerpc/platforms/powernv/setup.c b/arch/powerpc/platforms/powernv/setup.c
> >> index db1ad1c..331838e 100644
> >> --- a/arch/powerpc/platforms/powernv/setup.c
> >> +++ b/arch/powerpc/platforms/powernv/setup.c
> >> @@ -25,6 +25,7 @@
> >> #include <linux/of.h>
> >> #include <linux/interrupt.h>
> >> #include <linux/bug.h>
> >> +#include <linux/pci.h>
> >>
> >> #include <asm/machdep.h>
> >> #include <asm/firmware.h>
> >> @@ -179,6 +180,30 @@ static int __init pnv_probe(void)
> >> return 1;
> >> }
> >>
> >> +static void pnv_pcibios_fixup_resources(struct pci_dev *pdev)
> >> +{
> >> + struct resource *r;
> >> + int i;
> >> +
> >> + /*
> >> + * Aligning resources to PAGE_SIZE in order to
> >> + * support "fast" path for PCI BAR access under VFIO
> >> + * which maps every BAR individually to the guest
> >> + * so BARs have to be PAGE aligned.
> >> + */
> >> + for (i = 0; i <= PCI_ROM_RESOURCE; i++) {
> >> + r = &pdev->resource[i];
> >> + if (!r->flags)
> >> + continue;
> >> + pr_debug("powernv: %s, aligning BAR#%d %llx..%llx",
> >> + pdev->dev.kobj.name, i, r->start, r->end);
> >> + r->end = PAGE_ALIGN(r->end - r->start + 1) - 1;
> >> + r->start = 0;
> >> + r->flags |= IORESOURCE_UNSET;
> >> + pr_debug(" to %llx..%llx\n", r->start, r->end);
> >> + }
> >> +}
> >> +
> >> define_machine(powernv) {
> >> .name = "PowerNV",
> >> .probe = pnv_probe,
> >> @@ -189,6 +214,7 @@ define_machine(powernv) {
> >> .progress = pnv_progress,
> >> .power_save = power7_idle,
> >> .calibrate_decr = generic_calibrate_decr,
> >> + .pcibios_fixup_resources= pnv_pcibios_fixup_resources,
> >> #ifdef CONFIG_KEXEC
> >> .kexec_cpu_down = pnv_kexec_cpu_down,
> >> #endif
> >
> >
>
>
^ permalink raw reply
* Re: [PATCH v7 1/8] Talitos: Support for async_tx XOR offload
From: Dan Williams @ 2012-09-05 1:19 UTC (permalink / raw)
To: Liu Qiang-B32616
Cc: Li Yang-R58472, arnd@arndb.de, vinod.koul@intel.com,
gregkh@linuxfoundation.org, Dave Jiang,
linux-kernel@vger.kernel.org, Phillips Kim-R1AAHA,
linux-crypto@vger.kernel.org, linuxppc-dev@lists.ozlabs.org,
davem@davemloft.net, herbert@gondor.apana.org.au
In-Reply-To: <BCB48C05FCE8BC4D9E61E841ECBE6DB70FFB67@039-SN2MPN1-011.039d.mgd.msft.net>
On Tue, Sep 4, 2012 at 5:28 AM, Liu Qiang-B32616 <B32616@freescale.com> wrote:
>> Will this engine be coordinating with another to handle memory copies?
>> The dma mapping code for async_tx/raid is broken when dma mapping
>> requests overlap or cross dma device boundaries [1].
>>
>> [1]: http://marc.info/?l=linux-arm-kernel&m=129407269402930&w=2
> Yes, it needs fsl-dma to handle memcpy copies.
> I read your link, the unmap address is stored in talitos hwdesc, the address will be unmapped when async_tx ack this descriptor, I know fsl-dma won't wait this ack flag in current kernel, so I fix it in fsl-dma patch 5/8. Do you mean that?
Unfortunately no. I'm open to other suggestions. but as far as I can
see it requires deeper changes to rip out the dma mapping that happens
in async_tx and the automatic unmapping done by drivers. It should
all be pushed to the client (md).
Currently async_tx hides hardware details from md such that it doesn't
even care if the operation is offloaded to hardware at all, but that
takes things too far. In the worst case an copy->xor chain handled by
multiple channels results in :
1/ dma_map(copy_chan...)
2/ dma_map(xor_chan...)
3/ <exec copy>
4/ dma_unmap(copy_chan...)
5/ <exec xor> <---initiated by the copy_chan
6/ dma_unmap(xor_chan...)
Step 2 violates the dma api since the buffers belong to the xor_chan
until unmap. Step 5 also causes the random completion context of the
copy channel to bleed into submission context of the xor channel which
is problematic. So the order needs to be:
1/ dma_map(copy_chan...)
2/ <exec copy>
3/ dma_unmap(copy_chan...)
4/ dma_map(xor_chan...)
5/ <exec xor> <--initiated by md in a static context
6/ dma_unmap(xor_chan...)
Also, if xor_chan and copy_chan lie with the same dma mapping domain
(iommu or parent device) then we can map the stripe once and skip the
extra maintenance for the duration of the chain of operations. This
dumps a lot of hardware details on md, but I think it is the only way
to get consistent semantics when arbitrary offload devices are
involved.
--
Dan
^ permalink raw reply
* [PATCH] powerpc-powernv: added tce_get callback for powernv platform
From: Alexey Kardashevskiy @ 2012-09-05 1:19 UTC (permalink / raw)
To: Benjamin Herrenschmidt
Cc: Alexey Kardashevskiy, linuxppc-dev, Paul Mackerras, david
The upcoming VFIO support requires a way to know which
entry in the TCE map is not empty in order to do cleanup
at QEMU exit/crash. This patch adds such functionality
to POWERNV platform code.
Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
arch/powerpc/platforms/powernv/pci.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/arch/powerpc/platforms/powernv/pci.c b/arch/powerpc/platforms/powernv/pci.c
index be3cfc5..05205cf 100644
--- a/arch/powerpc/platforms/powernv/pci.c
+++ b/arch/powerpc/platforms/powernv/pci.c
@@ -447,6 +447,11 @@ static void pnv_tce_free(struct iommu_table *tbl, long index, long npages)
pnv_tce_invalidate(tbl, tces, tcep - 1);
}
+static unsigned long pnv_tce_get(struct iommu_table *tbl, long index)
+{
+ return ((u64 *)tbl->it_base)[index - tbl->it_offset];
+}
+
void pnv_pci_setup_iommu_table(struct iommu_table *tbl,
void *tce_mem, u64 tce_size,
u64 dma_offset)
@@ -597,6 +602,7 @@ void __init pnv_pci_init(void)
ppc_md.pci_dma_dev_setup = pnv_pci_dma_dev_setup;
ppc_md.tce_build = pnv_tce_build;
ppc_md.tce_free = pnv_tce_free;
+ ppc_md.tce_get = pnv_tce_get;
ppc_md.pci_probe_mode = pnv_pci_probe_mode;
set_pci_dma_ops(&dma_iommu_ops);
--
1.7.10.4
^ permalink raw reply related
* Re: [RFC v8 PATCH 08/20] memory-hotplug: remove /sys/firmware/memmap/X sysfs
From: Wen Congyang @ 2012-09-05 1:41 UTC (permalink / raw)
To: Andrew Morton
Cc: linux-s390, linux-ia64, len.brown, linux-acpi, linux-sh, x86,
linux-kernel, cmetcalf, linux-mm, isimatu.yasuaki, paulus,
minchan.kim, kosaki.motohiro, rientjes, sparclinux,
Christoph Lameter, cl, linuxppc-dev, liuj97
In-Reply-To: <20120904161634.f1f9f693.akpm@linux-foundation.org>
At 09/05/2012 07:16 AM, Andrew Morton Wrote:
> On Mon, 03 Sep 2012 13:51:10 +0800
> Wen Congyang <wency@cn.fujitsu.com> wrote:
>
>>>> +static void release_firmware_map_entry(struct kobject *kobj)
>>>> +{
>>>> + struct firmware_map_entry *entry = to_memmap_entry(kobj);
>>>> + struct page *page;
>>>> +
>>>> + page = virt_to_page(entry);
>>>> + if (PageSlab(page) || PageCompound(page))
>>>
>>> That PageCompound() test looks rather odd. Why is this done?
>>
>> Liu Jiang and Christoph Lameter discussed how to find slab page
>> in this mail:
>> https://lkml.org/lkml/2012/7/6/333.
>
> Well, please add a code comment to release_firmware_map_entry() which
> fully explains these things.
>
> I see that Christoph and I agree: "It would be cleaner if memory
> hotplug had an indicator which allocation mechanism was used and would
> use the corresponding free action". You didn't respond to this
> suggestion when he made it, nor when I made it. What are your thoughts
> on this?
Hmm, I think it is better to use an indicator which allocation mechanism was
used. I will do it in the next version.
Thanks
Wen Congyang
^ permalink raw reply
* Re: [PATCH] clk: Make the generic clock API available by default
From: Benjamin Herrenschmidt @ 2012-09-05 2:03 UTC (permalink / raw)
To: Mark Brown
Cc: linux-mips, Guan Xuetao, Russell King, Arnd Bergmann,
linux-kernel, Ralf Baechle, Paul Mackerras, Haavard Skinnemoen,
linuxppc-dev, linux-arm-kernel, Hans-Christian Egtvedt
In-Reply-To: <1346186104-4083-1-git-send-email-broonie@opensource.wolfsonmicro.com>
On Tue, 2012-08-28 at 13:35 -0700, Mark Brown wrote:
> Rather than requiring platforms to select the generic clock API to make
> it available make the API available as a user selectable option unless the
> user either selects HAVE_CUSTOM_CLK (if they have their own implementation)
> or selects COMMON_CLK (if they depend on the generic implementation).
>
> All current architectures that HAVE_CLK but don't use the common clock
> framework have selects of HAVE_CUSTOM_CLK added.
>
> This allows drivers to use the generic API on platforms which have no need
> for the clock API at platform level.
>
> Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
> ---
For powerpc:
Acked-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cheers,
Ben.
^ permalink raw reply
* RE: [PATCH] usb: gadget: fsl_udc_core: do not immediatly prime STATUS for IN xfer
From: Chen Peter-B29397 @ 2012-09-05 2:10 UTC (permalink / raw)
To: Enrico Scholz, linux-usb@vger.kernel.org,
linuxppc-dev@lists.ozlabs.org
Cc: gregkh@linuxfoundation.org, Li Yang-R58472, balbi@ti.com
In-Reply-To: <1346777932-3362-1-git-send-email-enrico.scholz@sigma-chemnitz.de>
=20
>=20
> Because the fsl_udc_core driver shares one 'status_req' object for the
> complete ep0 control transfer, it is not possible to prime the final
> STATUS phase immediately after the IN transaction. E.g. ch9getstatus()
> executed:
>=20
> | req =3D udc->status_req;
> | ...
> | list_add_tail(&req->queue, &ep->queue);
> | if (ep0_prime_status(udc, EP_DIR_OUT))
> | ....
> | struct fsl_req *req =3D udc->status_req;
> | list_add_tail(&req->queue, &ep->queue);
>=20
> which corrupts the ep->queue list by inserting 'status_req' twice. This
> causes a kernel oops e.g. when 'lsusb -v' is executed on the host.
>=20
> Patch delays the final 'ep0_prime_status(udc, EP_DIR_OUT))' by moving it
> into the ep0 completion handler.
>=20
Enrico, thanks for pointing this problem.
As "prime STATUS phase immediately after the IN transaction" is followed
USB 2.0 spec, to fix this problem, it is better to add data_req for ep0.
In fact, it is already at FSL i.mx internal code, just still not mainlined.
Peter
=20
^ permalink raw reply
* RE: [PATCH] usb: gadget: fsl_udc_core: remove mapped flag
From: Chen Peter-B29397 @ 2012-09-05 2:17 UTC (permalink / raw)
To: Enrico Scholz, linux-usb@vger.kernel.org,
linuxppc-dev@lists.ozlabs.org
Cc: gregkh@linuxfoundation.org, Li Yang-R58472, balbi@ti.com
In-Reply-To: <1346779499-6085-1-git-send-email-enrico.scholz@sigma-chemnitz.de>
=20
> @@ -195,14 +195,13 @@ static void done(struct fsl_ep *ep, struct fsl_req
> *req, int status)
> dma_pool_free(udc->td_pool, curr_td, curr_td->td_dma);
> }
>=20
> - if (req->mapped) {
> + if (req->req.dma !=3D DMA_ADDR_INVALID) {
> dma_unmap_single(ep->udc->gadget.dev.parent,
> req->req.dma, req->req.length,
> ep_is_in(ep)
> ? DMA_TO_DEVICE
> : DMA_FROM_DEVICE);
> req->req.dma =3D DMA_ADDR_INVALID;
> - req->mapped =3D 0;
> } else
> dma_sync_single_for_cpu(ep->udc->gadget.dev.parent,
If the class driver has already mapped this address, the req->req.dma is no=
t
DMA_ADDR_INVALID either, in this case, the dma_sync_single_for_cpu is enoug=
h.
Peter
^ permalink raw reply
* Re: [PATCH 13/25] macintosh/mediabay: add a const qualifier
From: Benjamin Herrenschmidt @ 2012-09-05 2:40 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Arnd Bergmann, Rob Herring, kernel, linuxppc-dev,
linux-arm-kernel
In-Reply-To: <1343034810-3386-14-git-send-email-u.kleine-koenig@pengutronix.de>
On Mon, 2012-07-23 at 11:13 +0200, Uwe Kleine-König wrote:
> This prepares *of_device_id.data becoming const. Without this change
> the following warning would occur:
>
> drivers/macintosh/mediabay.c: In function 'media_bay_attach':
> drivers/macintosh/mediabay.c:589:11: warning: assignment discards 'const' qualifier from pointer target type [enabled by default]
>
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
Ack all of these assuming you test built (I didn't). Do you need me to
carry any of this via the powerpc tree ?
Cheers,
Ben.
> drivers/macintosh/mediabay.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/macintosh/mediabay.c b/drivers/macintosh/mediabay.c
> index 831d751..54bf584 100644
> --- a/drivers/macintosh/mediabay.c
> +++ b/drivers/macintosh/mediabay.c
> @@ -63,7 +63,7 @@ struct media_bay_info {
> int value_count;
> int timer;
> struct macio_dev *mdev;
> - struct mb_ops* ops;
> + const struct mb_ops* ops;
> int index;
> int cached_gpio;
> int sleeping;
^ 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