Re: [PATCH v0 2/2] Crypto: Talitos: Support for Async_tx XOR offload

From: hank peng
Date: Mon Nov 02 2009 - 19:44:10 EST

Next message: Huang Ying: "Re: [RFC 2/2] Use unified UUID/GUID definition in gfs2"
Previous message: Masami Hiramatsu: "Re: [PATCH -tip perf/probes 00/10] x86 insn decoder bugfixes andperf-probe syntax changes"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

I have tested this patch on my MPC8548 machine box, kernel version is
2.6.30. There is a problem.
#mdadm -C /dev/md0 -l5 -n3 /dev/sd{a,b,c}
Recovery can be done successfully, interrupts looks normal.
# cat /proc/interrupts
CPU0
16: 16091057 OpenPIC Level mvSata
17: 0 OpenPIC Level mvSata
18: 4 OpenPIC Level phy_interrupt, phy_interrupt
20: 39 OpenPIC Level fsldma-channel
21: 0 OpenPIC Level fsldma-channel
22: 0 OpenPIC Level fsldma-channel
23: 0 OpenPIC Level fsldma-channel
29: 241 OpenPIC Level eth0_tx
30: 1004692 OpenPIC Level eth0_rx
34: 0 OpenPIC Level eth0_er
35: 0 OpenPIC Level eth1_tx
36: 0 OpenPIC Level eth1_rx
40: 0 OpenPIC Level eth1_er
42: 73060 OpenPIC Level serial
43: 9854 OpenPIC Level i2c-mpc, i2c-mpc
45: 61264188 OpenPIC Level talitos
BAD: 0

Then, I plan to create a VG, but problem occured.
#pvcreate /dev/md0

After I input this command, system hangs there without any messages.

BTW, all is OK before using this patch.

2009/10/30 Dan Williams <dan.j.williams@xxxxxxxxx>:
> On Wed, Oct 14, 2009 at 11:41 PM, Vishnu Suresh <Vishnu@xxxxxxxxxxxxx> wrote:
> [..]
>> diff --git a/drivers/crypto/Kconfig b/drivers/crypto/Kconfig
>> index b08403d..343e578 100644
>> --- a/drivers/crypto/Kconfig
>> +++ b/drivers/crypto/Kconfig
>> @@ -192,6 +192,8 @@ config CRYPTO_DEV_TALITOS
>> Â Â Â Âselect CRYPTO_ALGAPI
>> Â Â Â Âselect CRYPTO_AUTHENC
>> Â Â Â Âselect HW_RANDOM
>> + Â Â Â select DMA_ENGINE
>> + Â Â Â select ASYNC_XOR
>
> You need only select DMA_ENGINE. ÂASYNC_XOR is selected by its users.
>
>> Â Â Â Âdepends on FSL_SOC
>> Â Â Â Âhelp
>> Â Â Â Â ÂSay 'Y' here to use the Freescale Security Engine (SEC)
>> diff --git a/drivers/crypto/talitos.c b/drivers/crypto/talitos.c
>> index c47ffe8..84819d4 100644
>> --- a/drivers/crypto/talitos.c
>> +++ b/drivers/crypto/talitos.c
> [..]
>> +static void talitos_process_pending(struct talitos_xor_chan *xor_chan)
>> +{
>> + Â Â Â struct talitos_xor_desc *desc, *_desc;
>> + Â Â Â unsigned long flags;
>> + Â Â Â int status;
>> +
>> + Â Â Â spin_lock_irqsave(&xor_chan->desc_lock, flags);
>> +
>> + Â Â Â list_for_each_entry_safe(desc, _desc, &xor_chan->pending_q, node) {
>> + Â Â Â Â Â Â Â status = talitos_submit(xor_chan->dev, &desc->hwdesc,
>> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â talitos_release_xor, desc);
>> + Â Â Â Â Â Â Â if (status != -EINPROGRESS)
>> + Â Â Â Â Â Â Â Â Â Â Â break;
>> +
>> + Â Â Â Â Â Â Â list_del(&desc->node);
>> + Â Â Â Â Â Â Â list_add_tail(&desc->node, &xor_chan->in_progress_q);
>> + Â Â Â }
>> +
>> + Â Â Â spin_unlock_irqrestore(&xor_chan->desc_lock, flags);
>> +}
>
> The driver uses spin_lock_bh everywhere else which is either a bug, or
> this code is being overly protective. ÂIn any event lockdep will
> rightly complain about this. ÂThe API and its users do not submit
> operations in hard-irq context.
>
>> +
>> +static void talitos_release_xor(struct device *dev, struct talitos_desc *hwdesc,
>> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â void *context, int error)
>> +{
>> + Â Â Â struct talitos_xor_desc *desc = 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);
>> + Â Â Â Â Â Â Â BUG();
>> + Â Â Â }
>> +
>> + Â Â Â xor_chan = 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 = desc->async_tx.cookie;
>> +
>> + Â Â Â callback = desc->async_tx.callback;
>> + Â Â Â callback_param = desc->async_tx.callback_param;
>> +
>> + Â Â Â if (callback) {
>> + Â Â Â Â Â Â Â spin_unlock_bh(&xor_chan->desc_lock);
>> + Â Â Â Â Â Â Â callback(callback_param);
>> + Â Â Â Â Â Â Â spin_lock_bh(&xor_chan->desc_lock);
>> + Â Â Â }
>
> You do not need to unlock to call the callback. ÂUsers of the API
> assume that they are called in bh context and that they are not
> allowed to submit new operations directly from the callback (this
> simplifies the descriptor cleanup routines in other drivers).
>
>> +
>> + Â Â Â 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);
>> +}
>
> It appears this routine is missing a call to dma_run_dependencies()?
> This is needed if another channel is handling memory copy offload. ÂI
> assume this is the case due to your other patch to fsldma. ÂSee
> iop_adma_run_tx_complete_actions(). ÂIf this xor resource can also
> handle copy operations that would be more efficient as it eliminates
> channel switching. ÂSee the ioatdma driver and its use of
> ASYNC_TX_DISABLE_CHANNEL_SWITCH.
>
>> +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(unlikely(len > TALITOS_MAX_DATA_LEN));
>> +
>> + Â Â Â xor_chan = container_of(chan, struct talitos_xor_chan, common);
>> +
>> + Â Â Â spin_lock_bh(&xor_chan->desc_lock);
>> + Â Â Â if (!list_empty(&xor_chan->free_desc)) {
>> + Â Â Â Â Â Â Â new = container_of(xor_chan->free_desc.next,
>> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Âstruct talitos_xor_desc, node);
>> + Â Â Â Â Â Â Â list_del(&new->node);
>> + Â Â Â } else {
>> + Â Â Â Â Â Â Â new = talitos_xor_alloc_descriptor(xor_chan, GFP_KERNEL);
>> + Â Â Â }
>> + Â Â Â 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);
>
> You can save some overhead in the fast path by moving this
> initialization to talitos_xor_alloc_descriptor.
>
>> +
>> + Â Â Â desc = &new->hwdesc;
>> + Â Â Â /* Set destination: Last pointer pair */
>> + Â Â Â to_talitos_ptr(&desc->ptr[6], dest);
>> + Â Â Â desc->ptr[6].len = cpu_to_be16(len);
>> + Â Â Â desc->ptr[6].j_extent = 0;
>> +
>> + Â Â Â /* Set Sources: End loading from second-last pointer pair */
>> + Â Â Â for (i = 5, j = 0; (j < src_cnt) && (i > 0); i--, j++) {
>> + Â Â Â Â Â Â Â to_talitos_ptr(&desc->ptr[i], src[j]);
>> + Â Â Â Â Â Â Â desc->ptr[i].len = cpu_to_be16(len);
>> + Â Â Â Â Â Â Â desc->ptr[i].j_extent = 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 >= 0; i--) {
>> + Â Â Â Â Â Â Â to_talitos_ptr(&desc->ptr[i], 0);
>> + Â Â Â Â Â Â Â desc->ptr[i].len = 0;
>> + Â Â Â Â Â Â Â desc->ptr[i].j_extent = 0;
>> + Â Â Â }
>> +
>> + Â Â Â desc->hdr = DESC_HDR_SEL0_AESU | DESC_HDR_MODE0_AESU_XOR
>> + Â Â Â Â Â Â Â Â Â | DESC_HDR_TYPE_RAID_XOR;
>> +
>> + Â Â Â new->async_tx.parent = NULL;
>> + Â Â Â new->async_tx.next = NULL;
>
> These fields are managed by the async_tx channel switch code. ÂNo need
> to manage it here.
>
>> + Â Â Â new->async_tx.cookie = 0;
>
> This is set below to -EBUSY, it's redundant to touch it here.
>
>> + Â Â Â async_tx_ack(&new->async_tx);
>
> This makes it impossible to attach a dependency to this operation.
> Not sure this is what you want.
>
> --
> Dan
> --
> To unsubscribe from this list: send the line "unsubscribe linux-raid" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at Âhttp://vger.kernel.org/majordomo-info.html
>

--
The simplest is not all best but the best is surely the simplest!
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Next message: Huang Ying: "Re: [RFC 2/2] Use unified UUID/GUID definition in gfs2"
Previous message: Masami Hiramatsu: "Re: [PATCH -tip perf/probes 00/10] x86 insn decoder bugfixes andperf-probe syntax changes"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]