Re: On gratuitous modularization

[Christian Franke]

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