Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII;
 format=flowed
Content-Transfer-Encoding: 7bit
Date:   Fri, 29 Apr 2022 11:44:49 +0200
From:   Michael Walle <michael@walle.cc>
To:     Pratyush Yadav <p.yadav@ti.com>
Cc:     Tudor Ambarus <tudor.ambarus@microchip.com>,
        Miquel Raynal <miquel.raynal@bootlin.com>,
        Richard Weinberger <richard@nod.at>,
        Vignesh Raghavendra <vigneshr@ti.com>,
        linux-kernel@vger.kernel.org, linux-mtd@lists.infradead.org
Subject: Re: [PATCH v2 2/2] mtd: spi-nor: expose internal parameters via
 debugfs
In-Reply-To: <20220429045731.sjhlkyxfxw4zvfxt@ti.com>
References: <20220428122840.1496876-1-michael@walle.cc>
 <20220428122840.1496876-2-michael@walle.cc>
 <20220429045731.sjhlkyxfxw4zvfxt@ti.com>
User-Agent: Roundcube Webmail/1.4.13
Message-ID: <f303280eea2e7a4bc2d14a5cd22727b1@walle.cc>
Precedence: bulk

Hi,

>> +#define SNOR_F_NAME(name) [ilog2(SNOR_F_##name)] = #name
>> +static const char *const snor_f_names[] = {
>> +	SNOR_F_NAME(HAS_SR_TB),
>> +	SNOR_F_NAME(NO_OP_CHIP_ERASE),
>> +	SNOR_F_NAME(BROKEN_RESET),
>> +	SNOR_F_NAME(4B_OPCODES),
>> +	SNOR_F_NAME(HAS_4BAIT),
>> +	SNOR_F_NAME(HAS_LOCK),
>> +	SNOR_F_NAME(HAS_16BIT_SR),
>> +	SNOR_F_NAME(NO_READ_CR),
>> +	SNOR_F_NAME(HAS_SR_TB_BIT6),
>> +	SNOR_F_NAME(HAS_4BIT_BP),
>> +	SNOR_F_NAME(HAS_SR_BP3_BIT6),
>> +	SNOR_F_NAME(IO_MODE_EN_VOLATILE),
>> +	SNOR_F_NAME(SOFT_RESET),
>> +	SNOR_F_NAME(SWP_IS_VOLATILE),
>> +};
>> +#undef SNOR_F_NAME
> 
> You said you would add a comment here. Changed your mind?

No, it just slipped through.

>> +void spi_nor_debugfs_register(struct spi_nor *nor)
>> +{
>> +	struct dentry *d;
>> +	int ret;
>> +
>> +	/* Create rootdir once. Will never be deleted again. */
>> +	if (!rootdir)
>> +		rootdir = debugfs_create_dir("spi-nor", NULL);
> 
> If I compile SPI NOR as module, I insmod it, rmmod it, and then insmod
> it again, I get:
> 
> 	[  360.623465] spi-nor spi0.0: mt35xu512aba (65536 Kbytes)
> 	[  360.623478] debugfs: Directory 'spi-nor' with parent '/' already 
> present!
> 	[  360.632237] spi-nor spi1.0: mt25qu512a (65536 Kbytes)
> 
> I guess when you remove the module, rootdir also gets destroyed, and
> then gets re-initialized on probing again. I am not familiar enough 
> with
> the debugfs APIs to suggest a fix though.

Thanks for testing. And yes, you are right. I've changed that code
quite a few times for this damn subdirectory. But it seems I didn't
get it right. Usually, it's created in the module_init() but since
we just have an implicit one (and I don't really want to change that),
that's not an option. Some subsystems don't create a subdirectory at
all, which doesn't appeal to me.

I'll first lookup if the directory is already there, if it is, use it,
if not, create it. That should work. FWIW, the mvpp2 network card driver
does it too.

-michael