Re: [PATCH v2 06/12] drivers/soc: Add Aspeed XDMA Engine Driver

[Eddie James <eajames@linux.vnet.ibm.com>]

On 12/11/19 10:52 PM, Andrew Jeffery wrote:
>
> On Thu, 12 Dec 2019, at 07:09, Eddie James wrote:
>> On 12/10/19 9:47 PM, Andrew Jeffery wrote:
>>> On Fri, 6 Dec 2019, at 03:45, Eddie James wrote:
>>>> +
>>>> +static unsigned int aspeed_xdma_ast2600_set_cmd(struct aspeed_xdma *ctx,
>>>> +						struct aspeed_xdma_op *op,
>>>> +						u32 bmc_addr)
>>>> +{
>>>> +	u64 cmd = XDMA_CMD_AST2600_CMD_IRQ_BMC |
>>>> +		(op->direction ? XDMA_CMD_AST2600_CMD_UPSTREAM : 0);
>>>> +	unsigned int line_size;
>>>> +	unsigned int nidx = (ctx->cmd_idx + 1) % XDMA_NUM_CMDS;
>>>> +	unsigned int line_no = 1;
>>>> +	unsigned int pitch = 1;
>>>> +	struct aspeed_xdma_cmd *ncmd =
>>>> +		&(((struct aspeed_xdma_cmd *)ctx->cmdq)[ctx->cmd_idx]);
>>>> +
>>>> +	if ((op->host_addr + op->len) & 0xffffffff00000000ULL)
>>> Do we know that this won't wrap?
>>
>> No, but I assume it would be a bad transfer anyway at that point?
> But what happens as a consequence? We would have a 64 bit address
> but wouldn't enable 64bit addressing, so presumably the hardware
> would only use the bottom 32 bits of the address?
>
> Things could get weird yes?
>
> Or is there some failure that would occur before we trigger the transfer?
> Is that what you're depending on?


OK, I'll handle it then.


>
>>>> +
>>>> +static void aspeed_xdma_done(struct aspeed_xdma *ctx, bool error)
>>>> +{
>>>> +	if (ctx->current_client) {
>>>> +		ctx->current_client->error = error;
>>>> +		ctx->current_client->in_progress = false;
>>>> +		ctx->current_client = NULL;
>>> You need to take start_lock before writing these members to ensure the
>>> writes are not reordered across acquisition of start_lock in
>>> aspeed_xdma_start() above, unless there's some other guarantee of that?
>>
>> Unless we get spurious interrupts (as in, the xdma interrupt fires with
>> no transfer started, and somehow the correct status bits are set), it's
>> not possible to execute this at the same time as aspeed_xdma_start(). So
>> I did not try and lock here. Do you think it's worth locking for that
>> situation?
>>
> Why is it worth not locking? How is it correct? To answer that way we invoke
> all kinds of reasoning about multi-processing (interrupt handled on one core
> while aspeed_xdma_start() is executing on another), value visibility and
> instruction reordering (though as it happens the 2400, 2500 and 2600 are all
> in-order). We'll trip ourselves up if there is eventually a switch to out-of-order
> execution where the writes might be reordered and delayed until after
> start_lock has been acquired in aspeed_xdma_start() by a subseqent transfer.
> This line of reasoning is brittle exploitation of properties of the currently used
> cores for no benefit. Finishing the DMA op isn't a hot path where you might
> want to take some of these risks for performance, so we have almost zero
> care for lock contention but we must always be concerned about correctness.
>
> We avoid invoking all of those questions by acquiring the lock.


OK, I'll refactor to lock it.


>
>>>> +
>>>> +	ctx->vga_pool = devm_gen_pool_create(dev, ilog2(PAGE_SIZE), -1, NULL);
>>>> +	if (!ctx->vga_pool) {
>>>> +		dev_err(dev, "Failed to setup genalloc pool.\n");
>>>> +		return -ENOMEM;
>>>> +	}
>>>> +
>>>> +	rc = of_property_read_u32_array(dev->of_node, "vga-mem", vgamem, 2);
>>> As mentioned, this could be any reserved memory range. Also can't we get it as
>>> a resource rather than parsing a u32 array? Not sure if there's an advantage
>>> but it feels like a better representation.
>>
>> That doesn't work unfortunately because the VGA memory is not mapped and
>> the reserved memory subsystem fails to find it.
> Fair enough.
>
>>>> +
>>>> +	regmap_update_bits(sdmc, SDMC_REMAP, ctx->chip->sdmc_remap,
>>>> +			   ctx->chip->sdmc_remap);
>>> I disagree with doing this. As mentioned on the bindings it should be up to
>>> the platform integrator to ensure that this is configured appropriately.
>>
>> Probably so, but then how does one actually configure that elsewhere? Do
>> you mean add code to the edac driver (and add support for the ast2600)
>> to read some dts properties to set it?
> Right. That's where I was going. I don't expect you to do that as part of this
> patch series, but if you could separate this code out into separate patches
> (dealing with the sdmc property in the devicetree binding as well) we can at
> least concentrate on getting the core XDMA driver in and work out how to
> move forward with configuring the memory controller later.


Yea... my concern is that then we end up with a driver upstream that 
doesn't actually work. Same concern with the reset thing you mentioned 
below.


Thanks,

Eddie


>
>>>> +/*
>>>> + * aspeed_xdma_direction
>>>> + *
>>>> + * ASPEED_XDMA_DIRECTION_DOWNSTREAM: transfers data from the host to the BMC
>>>> + *
>>>> + * ASPEED_XDMA_DIRECTION_UPSTREAM: transfers data from the BMC to the host
>>>> + *
>>>> + * ASPEED_XDMA_DIRECTION_RESET: resets the XDMA engine
>>>> + */
>>>> +enum aspeed_xdma_direction {
>>>> +	ASPEED_XDMA_DIRECTION_DOWNSTREAM = 0,
>>>> +	ASPEED_XDMA_DIRECTION_UPSTREAM,
>>>> +	ASPEED_XDMA_DIRECTION_RESET,
>>> I still think having a reset action as part of the direction is a bit funky. Can you maybe
>>> put that in a separate patch so we can debate it later?
>>
>> I can, but I'm fairly convinced this is the cleanest way to add the
>> reset functionality.
>>
> Right, but if you separate it out you'll get my reviewed-by on the core XDMA
> patches much quicker :) You can convince me about it in slow-time
>
> Cheers,
>
> Andrew