linux-kernel - Re: [RFC][PATCH V3] axi: add AXI bus driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 12 Apr 2011 07:41:18 +0200
From:	Rafał Miłecki <zajec5@...il.com>
To:	Greg KH <greg@...ah.com>
Cc:	linux-wireless@...r.kernel.org,
	"John W. Linville" <linville@...driver.com>,
	George Kashperko <george@...u.edu.ua>,
	Arnd Bergmann <arnd@...db.de>,
	Russell King <rmk@....linux.org.uk>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	b43-dev@...ts.infradead.org,
	Michael Büsch <mb@...sch.de>,
	linuxdriverproject <devel@...uxdriverproject.org>,
	Andy Botting <andy@...ybotting.com>,
	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>,
	Larry Finger <Larry.Finger@...inger.net>
Subject: Re: [RFC][PATCH V3] axi: add AXI bus driver

2011/4/12 Greg KH <greg@...ah.com>:
> On Tue, Apr 12, 2011 at 12:45:33AM +0200, Rafał Miłecki wrote:
>> 2011/4/12 Greg KH <greg@...ah.com>:
>> > On Tue, Apr 12, 2011 at 12:12:47AM +0200, Rafał Miłecki wrote:
>> >> 2011/4/11 Greg KH <greg@...ah.com>:
>> >> > On Mon, Apr 11, 2011 at 11:36:39PM +0200, Rafał Miłecki wrote:
>> >> >> 2011/4/11 Greg KH <greg@...ah.com>:
>> >> >> > Please read the documentation for how to do this properly.  I find it
>> >> >> > really hard to believe that you wrote that comment instead of putting in
>> >> >> > the 2 lines of code required for this function.
>> >> >> >
>> >> >> > Especially as-it-is, your code does not work properly and leaks memory
>> >> >> > badly.  Why would you do that on purpose?
>> >> >>
>> >> >> I tried to read some documentation about this.
>> >> >>
>> >> >> 1) driver-mode/device.txt says only that:
>> >> >> > Callback to free the device after all references have
>> >> >> > gone away. This should be set by the allocator of the
>> >> >> > device (i.e. the bus driver that discovered the device).
>> >> >> I *really* do not know how my driver should "free" core on AXI bus.
>> >> >
>> >> > The structure that you have created, added to the bus, is now ready to
>> >> > have its memory freed.  So free it.
>> >> >
>> >> > This usually means something like:
>> >> >        struct my_obj = to_my_obj(dev);
>> >> >        kfree(my_obj);
>> >> > in the release function.
>> >>
>> >> I register core->dev to the bus (I set core->dev.bus and
>> >> core->dev.parent, is that what you mean?). This core->dev is "struct
>> >> dev" embedded in "struct axi_device". By embedded I mean it is *not* a
>> >> pointer, I do not alloc it, it's part of the "struct axi_device".
>> >
>> > That is exactly as it should be.
>> >
>> > Then in your release function, free the struct axi_device.  It's that
>> > simple.  To try to free it before then would be wrong and cause
>> > problems.
>>
>> This is because it is defined as:
>> struct axi_device cores[AXI_MAX_NR_CORES];
>
> No way, seriously?
>
> You can't do that, no static struct devices please.  Make these dynamic
> and everything will be fine.  The -mm tree used to have a huge warning
> if you ever tried to register a statically allocated struct, but that
> didn't really work out, but would have saved you a lot of time here,
> sorry.
>
> So dynamically allocate the structures and you will be fine.

Well, I saw that along kernel, I had no idea there is anything wrong
about this. It seems more ppl do not know about this:
struct radeon_ib	ibs[RADEON_IB_POOL_SIZE];
struct radeon_pm_clock_info clock_info[8];
struct radeon_pm_profile profiles[PM_PROFILE_MAX];
struct radeon_surface_reg surface_regs[RADEON_GEM_MAX_SURFACES];
struct radeon_i2c_chan *i2c_bus[RADEON_MAX_I2C_BUS];

struct b43_key key[B43_NR_GROUP_KEYS * 2 + B43_NR_PAIRWISE_KEYS];

struct ssb_device devices[SSB_MAX_NR_CORES];
I guess I could fine more examples by simple grepping .h files.

Is there some guide around with things like this we should avoid?
checkpatch does no catch this, so maybe just some manual? Could you
point me to it?

-- 
Rafał
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/