Re: On gratuitous modularization

[Christian Franke <Christian.Franke@t-online.de>]

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:
>> http://lists.gnu.org/archive/html/grub-devel/2009-02/msg00091.html
>>
>> 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