Message-ID: <20201211080301.GC1781038@piout.net>
Date:   Fri, 11 Dec 2020 09:03:01 +0100
From:   Alexandre Belloni <alexandre.belloni@...tlin.com>
To:     Markus Elfring <Markus.Elfring@....de>
Cc:     linux-mmc@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        Ludovic Desroches <ludovic.desroches@...rochip.com>,
        Nicolas Ferre <nicolas.ferre@...rochip.com>,
        Ulf Hansson <ulf.hansson@...aro.org>,
        LKML <linux-kernel@...r.kernel.org>,
        kernel-janitors@...r.kernel.org,
        Colin Ian King <colin.king@...onical.com>,
        Dan Carpenter <dan.carpenter@...cle.com>
Subject: Re: mmc: atmel-mci: Reduce scope for the variable “slot” in atmci_request_end()

On 11/12/2020 07:34:41+0100, Markus Elfring wrote:
> >> How do you think about a patch like “staging: speakup: remove redundant initialization
> >> of pointer p_key” for comparison?
> >> https://lore.kernel.org/patchwork/patch/1199128/
> >> https://lore.kernel.org/driverdev-devel/20200223153954.420731-1-colin.king@canonical.com/
> >>
> >> Would you tolerate to omit the initialisation for the variable “slot”?
> >
> > If you were able to provide one good technical reason.
> 
> I find that the positions of variable definitions (and similar assignments) influence
> the generation of executable code.
> 

And you are wrong, it doesn't. Before:

c044a0f0 <atmci_request_end>:
{
c044a0f0:	e92d4070 	push	{r4, r5, r6, lr}
c044a0f4:	e1a04000 	mov	r4, r0
	WARN_ON(host->cmd || host->data);
c044a0f8:	e5902024 	ldr	r2, [r0, #36]	; 0x24
{
c044a0fc:	e1a06001 	mov	r6, r1
	struct mmc_host		*prev_mmc = host->cur_slot->mmc;
c044a100:	e590301c 	ldr	r3, [r0, #28]
	WARN_ON(host->cmd || host->data);
c044a104:	e3520000 	cmp	r2, #0
	struct mmc_host		*prev_mmc = host->cur_slot->mmc;
c044a108:	e5935000 	ldr	r5, [r3]
	WARN_ON(host->cmd || host->data);
c044a10c:	0a00002d 	beq	c044a1c8 <atmci_request_end+0xd8>
c044a110:	e3000000 	movw	r0, #0
			c044a110: R_ARM_MOVW_ABS_NC	.LC0
c044a114:	e3a03000 	mov	r3, #0
c044a118:	e3400000 	movt	r0, #0
			c044a118: R_ARM_MOVT_ABS	.LC0
c044a11c:	e3a02009 	mov	r2, #9
c044a120:	e300161c 	movw	r1, #1564	; 0x61c
c044a124:	ebfffffe 	bl	0 <warn_slowpath_fmt>
			c044a124: R_ARM_CALL	warn_slowpath_fmt
	del_timer(&host->timer);
c044a128:	e28400a4 	add	r0, r4, #164	; 0xa4
c044a12c:	ebfffffe 	bl	0 <del_timer>
			c044a12c: R_ARM_CALL	del_timer
	if (host->need_clock_update) {
c044a130:	e5d430a0 	ldrb	r3, [r4, #160]	; 0xa0
c044a134:	e3530000 	cmp	r3, #0
c044a138:	0a000005 	beq	c044a154 <atmci_request_end+0x64>
		atmci_writel(host, ATMCI_MR, host->mode_reg);
c044a13c:	e59420b8 	ldr	r2, [r4, #184]	; 0xb8
c044a140:	e5943000 	ldr	r3, [r4]
	asm volatile("str %1, %0"
c044a144:	e5832004 	str	r2, [r3, #4]
		if (host->caps.has_cfg_reg)
c044a148:	e5d420da 	ldrb	r2, [r4, #218]	; 0xda
c044a14c:	e3520000 	cmp	r2, #0
c044a150:	1a000019 	bne	c044a1bc <atmci_request_end+0xcc>
	host->cur_slot->mrq = NULL;
c044a154:	e594101c 	ldr	r1, [r4, #28]
	return READ_ONCE(head->next) == head;
c044a158:	e1a03004 	mov	r3, r4
c044a15c:	e3a02000 	mov	r2, #0
c044a160:	e5812010 	str	r2, [r1, #16]
	host->mrq = NULL;
c044a164:	e5842020 	str	r2, [r4, #32]
c044a168:	e5b31098 	ldr	r1, [r3, #152]!	; 0x98
	if (!list_empty(&host->queue)) {
c044a16c:	e1510003 	cmp	r1, r3
		host->state = STATE_IDLE;
c044a170:	05842094 	streq	r2, [r4, #148]	; 0x94
	if (!list_empty(&host->queue)) {
c044a174:	0a00000c 	beq	c044a1ac <atmci_request_end+0xbc>
		slot = list_entry(host->queue.next,
c044a178:	e5943098 	ldr	r3, [r4, #152]	; 0x98


After:

c044a0f0 <atmci_request_end>:
{
c044a0f0:	e92d4070 	push	{r4, r5, r6, lr}
c044a0f4:	e1a04000 	mov	r4, r0
	WARN_ON(host->cmd || host->data);
c044a0f8:	e5902024 	ldr	r2, [r0, #36]	; 0x24
{
c044a0fc:	e1a06001 	mov	r6, r1
	struct mmc_host		*prev_mmc = host->cur_slot->mmc;
c044a100:	e590301c 	ldr	r3, [r0, #28]
	WARN_ON(host->cmd || host->data);
c044a104:	e3520000 	cmp	r2, #0
	struct mmc_host		*prev_mmc = host->cur_slot->mmc;
c044a108:	e5935000 	ldr	r5, [r3]
	WARN_ON(host->cmd || host->data);
c044a10c:	0a00002d 	beq	c044a1c8 <atmci_request_end+0xd8>
c044a110:	e3000000 	movw	r0, #0
			c044a110: R_ARM_MOVW_ABS_NC	.LC0
c044a114:	e3a03000 	mov	r3, #0
c044a118:	e3400000 	movt	r0, #0
			c044a118: R_ARM_MOVT_ABS	.LC0
c044a11c:	e3a02009 	mov	r2, #9
c044a120:	e300161b 	movw	r1, #1563	; 0x61b
c044a124:	ebfffffe 	bl	0 <warn_slowpath_fmt>
			c044a124: R_ARM_CALL	warn_slowpath_fmt
	del_timer(&host->timer);
c044a128:	e28400a4 	add	r0, r4, #164	; 0xa4
c044a12c:	ebfffffe 	bl	0 <del_timer>
			c044a12c: R_ARM_CALL	del_timer
	if (host->need_clock_update) {
c044a130:	e5d430a0 	ldrb	r3, [r4, #160]	; 0xa0
c044a134:	e3530000 	cmp	r3, #0
c044a138:	0a000005 	beq	c044a154 <atmci_request_end+0x64>
		atmci_writel(host, ATMCI_MR, host->mode_reg);
c044a13c:	e59420b8 	ldr	r2, [r4, #184]	; 0xb8
c044a140:	e5943000 	ldr	r3, [r4]
	asm volatile("str %1, %0"
c044a144:	e5832004 	str	r2, [r3, #4]
		if (host->caps.has_cfg_reg)
c044a148:	e5d420da 	ldrb	r2, [r4, #218]	; 0xda
c044a14c:	e3520000 	cmp	r2, #0
c044a150:	1a000019 	bne	c044a1bc <atmci_request_end+0xcc>
	host->cur_slot->mrq = NULL;
c044a154:	e594101c 	ldr	r1, [r4, #28]
	return READ_ONCE(head->next) == head;
c044a158:	e1a03004 	mov	r3, r4
c044a15c:	e3a02000 	mov	r2, #0
c044a160:	e5812010 	str	r2, [r1, #16]
	host->mrq = NULL;
c044a164:	e5842020 	str	r2, [r4, #32]
c044a168:	e5b31098 	ldr	r1, [r3, #152]!	; 0x98
	if (!list_empty(&host->queue)) {
c044a16c:	e1510003 	cmp	r1, r3
		host->state = STATE_IDLE;
c044a170:	05842094 	streq	r2, [r4, #148]	; 0x94
	if (!list_empty(&host->queue)) {
c044a174:	0a00000c 	beq	c044a1ac <atmci_request_end+0xbc>
		struct atmel_mci_slot *slot = list_entry(host->queue.next,
c044a178:	e5943098 	ldr	r3, [r4, #152]	; 0x98


Do you realize your patch is just unnecessary churn now?

Is it too difficult for you to actually compile the driver and look
at the changes before submitting patches?


-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Powered by blists - more mailing lists