[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: On gratuitous modularization

From: Christian Franke
Subject: Re: On gratuitous modularization
Date: Sun, 07 Feb 2010 13:01:23 +0100
User-agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv: Gecko/20100104 SeaMonkey/2.0.2

Vladimir 'φ-coder/phcoder' Serbinenko wrote:
Christian Franke wrote:
Hi Robert,

Robert Millan wrote:

Please be careful when adding modules.  I see that too often new modules
are added without any real need to host this code separately.

kern/disk.c:        struct grub_disk_ata_pass_through_parms *);

this seems unnecessary.  ata_pthru is very small.  If it's only used
by hdparm,
why not just merge it?  This also avoids the additional code in kernel.

The module ata_pthru.mod exists only to keep ata.mod small, see:

Keeping the ata.mod specific pass-through function separate from
hdparm.mod was intentional. Merging this function into hdparm.mod
would only make sense if ata.mod will the only ATA access module with
pass-through functionality in the future. Hdparm.mod would then depend
on ata.mod. A 'hdparm -h' would load ata.mod and disable biosdisk access.

I hope we will eventually have an ahci.mod :-)

Are the parameters of current ata_pass_through ATA-specific or would
they be the same on AHCI?

Like e.g. the Linux HDIO_DRIVE_TASKFILE ioctl, the ata_pass_through() parameters are not controller or transport specific. It should work with any possible future implementation of ATA pass-through functionality, e.g.
- native AHCI driver
- other native drivers
- SAT (ANSI INCITS 431-2007) ATA pass through command for:
-- SATA drives behind USB-bridges with SAT support, or:
-- SATA drives in SAS enclosures

Merging ata_pthru.mod into hdparm.mod would make hdparm specific to one (outdated) class of controllers: the traditional IDE-controller (T13/1510D).

BTW: I agree that using a global function pointer
'grub_disk_ata_pass_through' is a hack. A cleaner design would be
possible with a grub_disk_dev.ioctl(.) call.

Such an ioctl() call would IMO be the best way to handle such device class specific extra functionality.

Christian Franke

reply via email to

[Prev in Thread] Current Thread [Next in Thread]